[GitHub] [drill] arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes for union and repeated list accessors
arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes
for union and repeated list accessors
URL: https://github.com/apache/drill/pull/2018#discussion_r397109336
##
File path:
exec/vector/src/main/java/org/apache/drill/exec/record/metadata/VariantColumnMetadata.java
##
@@ -145,16 +147,28 @@ public ColumnMetadata cloneEmpty() {
@Override
public ColumnMetadata copy() {
-// TODO Auto-generated method stub
-assert false;
-return null;
+return new VariantColumnMetadata(name, type, mode, variantSchema.copy());
}
@Override
public VariantMetadata variantSchema() {
return variantSchema;
}
+ @JsonProperty("type")
Review comment:
Remove json property annotation as it will be present in parent class.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes for union and repeated list accessors
arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes
for union and repeated list accessors
URL: https://github.com/apache/drill/pull/2018#discussion_r397109112
##
File path:
exec/vector/src/main/java/org/apache/drill/exec/record/metadata/AbstractColumnMetadata.java
##
@@ -295,12 +295,6 @@ public String toString() {
.toString();
}
- @JsonProperty("type")
- @Override
- public String typeString() {
-return majorType().toString();
- }
-
Review comment:
Please don't remove this method but leave it as abstract with json property
annotation:
```
@JsonProperty("type")
@Override
public abstract String typeString();
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes for union and repeated list accessors
arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes
for union and repeated list accessors
URL: https://github.com/apache/drill/pull/2018#discussion_r396333194
##
File path:
exec/vector/src/main/java/org/apache/drill/exec/record/metadata/VariantColumnMetadata.java
##
@@ -96,11 +134,13 @@ public StructureType structureType() {
public boolean isVariant() { return true; }
@Override
- public boolean isArray() { return type() == MinorType.LIST; }
+ public boolean isArray() {
+return super.isArray() || type() == MinorType.LIST;
+ }
@Override
public ColumnMetadata cloneEmpty() {
-return new VariantColumnMetadata(name, type, variantSchema.cloneEmpty());
+return new VariantColumnMetadata(name, type, mode, new VariantSchema());
Review comment:
@paul-rogers did you have a chance to address code review comment?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes for union and repeated list accessors
arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes
for union and repeated list accessors
URL: https://github.com/apache/drill/pull/2018#discussion_r391479369
##
File path:
exec/vector/src/main/java/org/apache/drill/exec/record/metadata/VariantColumnMetadata.java
##
@@ -96,11 +134,13 @@ public StructureType structureType() {
public boolean isVariant() { return true; }
@Override
- public boolean isArray() { return type() == MinorType.LIST; }
+ public boolean isArray() {
+return super.isArray() || type() == MinorType.LIST;
+ }
@Override
public ColumnMetadata cloneEmpty() {
-return new VariantColumnMetadata(name, type, variantSchema.cloneEmpty());
+return new VariantColumnMetadata(name, type, mode, new VariantSchema());
Review comment:
Well, it turned out the `typeString` is also used during column metadata
serialization / deserialization. Type is serialized in schematic type to
consume less space, plus it's also stored in the same way in Metastore (see
org.apache.drill.exec.record.metadata.AbstractColumnMetadata#createColumnMetadata).
So currently ser / de for `VariantColumnMetadata` will fail. Since you are
planning to use it in JSON, we need to ensure this class can be properly
serialized and deserialized. To ensure this we need to do two things:
1. update `typeString` (I propose we do this in this PR).
2. update schema parser (I can do it in separate PR).
Back to `typeString`:
1. If `VariantColumnMetadata` is based on `UNION`, we can describe it as
`UNION` without types but we'll lose all types defined in `VariantSchema` or we
can describe it as UNION and preserve types. The question is if
we need types during ser / de?
2. Similar case when `VariantColumnMetadata` is based on `LIST`, we can
describe it as ARRAY without types or with types ARRAY>.
Since you know more about unions, I'll leave to you the final decision on
how schematically union types should be represented, afterwards I'll adjust the
schema parser.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes for union and repeated list accessors
arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes
for union and repeated list accessors
URL: https://github.com/apache/drill/pull/2018#discussion_r390202680
##
File path:
exec/vector/src/main/java/org/apache/drill/exec/record/metadata/VariantColumnMetadata.java
##
@@ -96,11 +134,13 @@ public StructureType structureType() {
public boolean isVariant() { return true; }
@Override
- public boolean isArray() { return type() == MinorType.LIST; }
+ public boolean isArray() {
+return super.isArray() || type() == MinorType.LIST;
+ }
@Override
public ColumnMetadata cloneEmpty() {
-return new VariantColumnMetadata(name, type, variantSchema.cloneEmpty());
+return new VariantColumnMetadata(name, type, mode, new VariantSchema());
Review comment:
Also this class should override `public String typeString()` which is used
during ser / de.
For example, `UNION`. Not sure how to describe LIST
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes for union and repeated list accessors
arina-ielchiieva commented on a change in pull request #2018: DRILL-7633: Fixes
for union and repeated list accessors
URL: https://github.com/apache/drill/pull/2018#discussion_r390199915
##
File path:
exec/vector/src/main/java/org/apache/drill/exec/record/metadata/VariantColumnMetadata.java
##
@@ -96,11 +134,13 @@ public StructureType structureType() {
public boolean isVariant() { return true; }
@Override
- public boolean isArray() { return type() == MinorType.LIST; }
+ public boolean isArray() {
+return super.isArray() || type() == MinorType.LIST;
+ }
@Override
public ColumnMetadata cloneEmpty() {
-return new VariantColumnMetadata(name, type, variantSchema.cloneEmpty());
+return new VariantColumnMetadata(name, type, mode, new VariantSchema());
Review comment:
Could you please also implement `public ColumnMetadata copy() {` below since
yo are touching this class?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
