[BEAM-4461] Add Selected.flattenedSchema#10766
Conversation
There was a problem hiding this comment.
It took me a minute to understand what's going on with "selectFields" here and in process. I had no idea that the value on FieldAccess could be an id referencing a member FieldAccessDescriptor until I dug into the code. Let's document that as part of BEAM-9217 as well.
There was a problem hiding this comment.
Field names can contain an underscore, right? So in theory we could still end up with a non-unique name here if the schema had two nested fields like:
- layer1.layer2_layer3 -> "layer1_layer2_layer3"
- layer1.layer2.layer3 -> "layer1_layer2_layer3"
There was a problem hiding this comment.
yeah, though I think we should maybe make underscore a reserved character in fields. We also don't prevent users from putting dots in field names, but all sorts of things will break if they do.
There was a problem hiding this comment.
Added withFieldNameAs to allow the user to rename fields
There was a problem hiding this comment.
maybe keepMostNestedFieldName for consistency? up to you
There was a problem hiding this comment.
This should probably indicate it will concat field names with underscores
There was a problem hiding this comment.
bike-shed: Nothing in this file actually makes this a default, seems to be a hold-over from when the function was in Unnest
There was a problem hiding this comment.
Similar bike-shed: This mentions unnesting which isn't a thing after this change, maybe change to flattening?
There was a problem hiding this comment.
This looks nifty, but maybe made it into this PR by accident? I don't see it used anywhere
There was a problem hiding this comment.
I assume there's no concern with removing Unnest outright (rather than deprecating for a period and pointing to Select.flattenedSchema) since this is all Experimental?
There was a problem hiding this comment.
yeah - should be fine I think.
There was a problem hiding this comment.
Consider adding a test for this in FieldAccessDescriptorTest, and adding a docstring
|
retest this please |
2 similar comments
|
retest this please |
|
retest this please |
reuvenlax
force-pushed
the
make_unnest_great_again
branch
from
4618ae8 to
dca23a2
Compare
2 participants
R: @TheNeuralBit