You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rdblue (via GitHub)" <gi...@apache.org> on 2023/04/23 22:38:34 UTC
[GitHub] [iceberg] rdblue opened a new pull request, #7417: Views: Fix SQL view representation field name
rdblue opened a new pull request, #7417:
URL: https://github.com/apache/iceberg/pull/7417
This fixes the field name for view column descriptions and has a few other minor updates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7417: Views: Fix SQL view representation field name
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7417:
URL: https://github.com/apache/iceberg/pull/7417#discussion_r1176139576
##########
core/src/main/java/org/apache/iceberg/view/ViewVersionParser.java:
##########
@@ -80,22 +80,7 @@ static ViewVersion fromJson(JsonNode node) {
long timestamp = JsonUtil.getLong(TIMESTAMP_MS, node);
Map<String, String> summary = JsonUtil.getStringMap(SUMMARY, node);
Preconditions.checkArgument(
- summary != null, "Cannot parse view version with missing required field: %s", SUMMARY);
-
- String operation = null;
- ImmutableMap.Builder<String, String> versionSummary = ImmutableMap.builder();
- for (Map.Entry<String, String> summaryEntry : summary.entrySet()) {
- if (summaryEntry.getKey().equals(OPERATION)) {
- operation = summaryEntry.getValue();
- } else {
- versionSummary.put(summaryEntry.getKey(), summaryEntry.getValue());
- }
- }
-
- Preconditions.checkArgument(
- operation != null,
- "Cannot parse view version summary with missing required field: %s",
- OPERATION);
+ summary.containsKey(OPERATION), "Invalid view version summary, missing %s", OPERATION);
Review 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 https://github.com/apache/iceberg/pull/7428 to move the check to `ViewVersion`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7417: Views: Fix SQL view representation field name
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7417:
URL: https://github.com/apache/iceberg/pull/7417#discussion_r1175704309
##########
core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java:
##########
@@ -35,7 +35,7 @@ class SQLViewRepresentationParser {
private static final String DEFAULT_CATALOG = "default-catalog";
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";
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #7417: Views: Fix SQL view representation field name
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #7417:
URL: https://github.com/apache/iceberg/pull/7417#discussion_r1175523847
##########
core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java:
##########
@@ -35,7 +35,7 @@ class SQLViewRepresentationParser {
private static final String DEFAULT_CATALOG = "default-catalog";
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";
Review Comment:
Replied [here](https://github.com/apache/iceberg/pull/7416#discussion_r1175521596) 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #7417: Views: Fix SQL view representation field name
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #7417:
URL: https://github.com/apache/iceberg/pull/7417#discussion_r1175853423
##########
core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java:
##########
@@ -35,7 +35,7 @@ class SQLViewRepresentationParser {
private static final String DEFAULT_CATALOG = "default-catalog";
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-comments";
Review Comment:
FIELD_COMMENTS for the constant name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #7417: Views: Fix SQL view representation field name
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #7417:
URL: https://github.com/apache/iceberg/pull/7417#issuecomment-1519184541
@amogh-jahagirdar can you take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7417: Views: Fix SQL view representation field name
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7417:
URL: https://github.com/apache/iceberg/pull/7417#discussion_r1175867129
##########
core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java:
##########
@@ -35,7 +35,7 @@ class SQLViewRepresentationParser {
private static final String DEFAULT_CATALOG = "default-catalog";
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-comments";
Review Comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 merged pull request #7417: Views: Fix SQL view representation field name
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 merged PR #7417:
URL: https://github.com/apache/iceberg/pull/7417
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #7417: Views: Fix SQL view representation field name
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #7417:
URL: https://github.com/apache/iceberg/pull/7417#discussion_r1175504805
##########
api/src/main/java/org/apache/iceberg/view/ViewVersion.java:
##########
@@ -65,5 +65,8 @@ public interface ViewVersion {
*
* @return the string operation which produced the view version
*/
- String operation();
+ @Value.Derived
+ default String operation() {
+ return summary().get("operation");
+ }
}
Review 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"
##########
core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java:
##########
@@ -35,7 +35,7 @@ class SQLViewRepresentationParser {
private static final String DEFAULT_CATALOG = "default-catalog";
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";
Review Comment:
Replied [here](https://github.com/apache/iceberg/pull/7416#discussion_r1175521596) 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 call this a comment. Let me know your thoughts on this though
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7417: Views: Fix SQL view representation field name
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7417:
URL: https://github.com/apache/iceberg/pull/7417#discussion_r1175866531
##########
core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java:
##########
@@ -35,7 +35,7 @@ class SQLViewRepresentationParser {
private static final String DEFAULT_CATALOG = "default-catalog";
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";
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #7417: Views: Fix SQL view representation field name
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #7417:
URL: https://github.com/apache/iceberg/pull/7417#issuecomment-1521143290
Thanks for the fix Ryan!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org