Views: Fix SQL view representation field name #7417
Conversation
|
@amogh-jahagirdar can you take a look? |
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks @rdblue, I replied with my rationale on why I prefer a spec change to call the field "field-comments" #7416 (comment) . The remaining cleanup looks great to me though, thanks!
| @Value.Derived | ||
| default String operation() { | ||
| return summary().get("operation"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice, Derived is way cleaner. We don't need any of the special parsing logic for operation and derived also prevents manually setting the operation field, so there is no ambiguity in which is the real "operation"
| private static final String DEFAULT_NAMESPACE = "default-namespace"; | ||
| private static final String FIELD_ALIASES = "field-aliases"; | ||
| private static final String FIELD_COMMENTS = "field-comments"; | ||
| private static final String FIELD_DOCS = "field-docs"; |
There was a problem hiding this comment.
Replied here as well, I think we should change the spec to be field-comments instead because most mechanisms for creating/altering the view (such as SQL) will refer to this as "comment". Let me know your thoughts on this though
There was a problem hiding this comment.
I guess docs would have some consistency with NestedField in Schema that has a doc field. When using SQL we also refer to that as comment, e.g. in Hive
ALTER TABLE test_change CHANGE a1 a1 INT COMMENT 'this is column a1';
So I am not too concerned with the naming choice here.
But even if we go with docs, should we use doc to be more consistent?
There was a problem hiding this comment.
I updated to comments. I think we can still make a few changes like this where needed and I agree that comments works better.
The original logic was to be like "doc" in the schema fields, but that's already there and this is a separate concept that has multiple fields. I don't think we're losing much by making it more similar to the SQL.
core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java
Outdated
Show resolved
Hide resolved
rdblue
force-pushed
the
views-minor-updates
branch
from
6e4c75c to
ed4091a
Compare
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Sweet, this looks great to me thanks @rdblue!
|
Thanks for the fix Ryan! |
| operation != null, | ||
| "Cannot parse view version summary with missing required field: %s", | ||
| OPERATION); | ||
| summary.containsKey(OPERATION), "Invalid view version summary, missing %s", OPERATION); |
There was a problem hiding this comment.
one issue with this is unfortunately that we're only checking for the existence of operation in the Parser. With Immutables you'd typically do this on the constructed object itself during initialization using a method annotated with @Value.Check to guarantee a consistent state.
For example, with the current version one could create a ViewVersion without an operation in the summary map, which would fail with a confusing error: java.lang.NullPointerException: operation.
@rdblue I have opened #7428 to move the check to ViewVersion
(cherry picked from commit 973676a)
4 participants
This fixes the field name for view column descriptions and has a few other minor updates.