You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/02/16 05:17:28 UTC
[GitHub] [calcite] my7ym opened a new pull request #1811: [CALCITE-3789]
Support validation of UNNEST multiple array columns like Presto
my7ym opened a new pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r380293431
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java
##########
@@ -73,7 +73,9 @@
/** Conformance value that instructs Calcite to use SQL semantics
* consistent with Microsoft SQL Server version 2008. */
- SQL_SERVER_2008;
+ SQL_SERVER_2008,
+
+ PRESTO;
Review comment:
That's right. I am also debating on this. I could delete it or rename it. But I think the conformance is only for SQL string validation, so whether or not DB is kinda fine. But it's your call.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym edited a comment on issue #1811: [CALCITE-3789]
Support validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
my7ym edited a comment on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-605682367
> > > We should also add a test case for at least one SqlToRelConverterTest.
> >
> >
> > Will do it in the future if needed. I am still unclear that how this should be represented as RelNode. (discussed in https://issues.apache.org/jira/browse/CALCITE-3787)
> > Is it OK for you?
> > EDIT: Some other reasons:
> >
> > 1. We still plan to communicate with Presto using SQL.
> > 2. RelNode -> SqlNode is not quite mature I believe, but I may be wrong there.
> > 3. The optimization enabled by RelNode is helpful. But not a high-pri since Presto will also optimize the internal execution plan.
>
> Usually we have 2 kinds of use cases for Calcite:
>
> * Generates RelNode/RexNode to adapter other downstream planners
> * Generates RelNode/RexNode and unparse it as special SQL dialect to interpretation through JDBC.
>
> Either way we should generates a working plan, that is why I suggest to add SqlToRel test. For more general, we should add operator test and SQL ITCase if we change an operator semantics. But for you case, I think a SqlToRelConverter test is okey.
I have added tests for it. But it needs changes on SqlToRelConverter.
The change is because usually for STRUCT, Uncollect will flatten the type but for this feature, it should not. This is also aligned with the registered type during validation.
I am not sure whether this should be the best way to implement it. Let me know. Thanks.
EDIT: If you think this change is large unnecessarily because we don't need SqlToRel conversion right now), I am happy to revert the SqlToRel conversion changes.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r392539522
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
Couldn’t you use !allowAliasUnnestColumns ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r384144943
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java
##########
@@ -73,7 +73,9 @@
/** Conformance value that instructs Calcite to use SQL semantics
* consistent with Microsoft SQL Server version 2008. */
- SQL_SERVER_2008;
+ SQL_SERVER_2008,
+
+ PRESTO;
Review comment:
Presto has its own SQL parser & validator & type system, so it counts as a 'dialect' (conformance) for these purposes.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r380286337
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
Great name. Much more concise. Will do.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r392329925
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
I think it looks even more confusing. If you read the code carefully enough, it is **NOT** allowAliasUnnestColumns. If you prefer the name change, I think `disallowAliasUnnestColumn` is more appropriate.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on issue #1811: [CALCITE-3789] Support
validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-597447584
> We should also add a test case for at least one SqlToRelConverterTest.
Will do it in the future if needed. I am still unclear that how this should be represented as RelNode. (discussed in https://issues.apache.org/jira/browse/CALCITE-3787)
Is it OK for you?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r403423858
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2047,8 +2047,15 @@ protected void convertFrom(
case AS:
call = (SqlCall) from;
- convertFrom(bb, call.operand(0));
- if (call.operandCount() > 2
+ SqlNode firstOperand = call.operand(0);
+
+ if (firstOperand.getKind() == SqlKind.UNNEST) {
+ convertUnnest(bb, (SqlCall) firstOperand, getFieldAliases(call));
+ } else {
+ convertFrom(bb, firstOperand);
+ }
Review comment:
The operator row type belongs to the Validator namespace, i think we should fix it there instead of `SqlToRelConverter`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r403423858
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2047,8 +2047,15 @@ protected void convertFrom(
case AS:
call = (SqlCall) from;
- convertFrom(bb, call.operand(0));
- if (call.operandCount() > 2
+ SqlNode firstOperand = call.operand(0);
+
+ if (firstOperand.getKind() == SqlKind.UNNEST) {
+ convertUnnest(bb, (SqlCall) firstOperand, getFieldAliases(call));
+ } else {
+ convertFrom(bb, firstOperand);
+ }
Review comment:
The operator row type belongs to the Validator namespace, i think we should fix it there instead of `SqlToRelConverter`. Maybe you should try `AliasNamespace`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r379969084
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java
##########
@@ -73,7 +73,9 @@
/** Conformance value that instructs Calcite to use SQL semantics
* consistent with Microsoft SQL Server version 2008. */
- SQL_SERVER_2008;
+ SQL_SERVER_2008,
+
+ PRESTO;
Review comment:
PRESTO is not really a db, it behaves more like a computation engine, i'm not sure if we should add a new conformance for it.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r399837690
##########
File path: core/src/test/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java
##########
@@ -155,6 +155,8 @@ public MockCatalogReaderSimple(RelDataTypeFactory typeFactory,
deptNestedTable.addColumn("NAME", fixture.varchar10Type);
deptNestedTable.addColumn("SKILL", fixture.skillRecordType);
deptNestedTable.addColumn("EMPLOYEES", fixture.empListType);
+ deptNestedTable.addColumn("ADMINS", fixture.varchar5ArrayType);
+ deptNestedTable.addColumn("OFFICES", fixture.rectilinearPeekCoordMultisetType);
Review comment:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r403423360
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -1304,7 +1304,31 @@ public final Sql sql(String sql) {
final String sql = "select d.deptno, e2.empno\n"
+ "from dept_nested as d,\n"
+ " UNNEST(d.employees) as e2(empno, y, z)";
- sql(sql).with(getExtendedTester()).ok();
+ sql(sql).ok();
+ }
+
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-3789">[CALCITE-3789]
+ * Support validation of UNNEST multiple array columns like Presto</a>.
+ */
+ @Test public void testAliasUnnestArrayPlanWithSingleColumn() {
+ final String sql = "select d.deptno, employee.empno\n"
+ + "from dept_nested_expanded as d,\n"
+ + " UNNEST(d.employees) as t(employee)";
+ sql(sql).conformance(SqlConformanceEnum.PRESTO).decorrelate(false).ok();
Review comment:
It is, you should fix the `RelStructuredTypeFlattener`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404203965
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java
##########
@@ -88,6 +88,7 @@
* {@link SqlConformanceEnum#BABEL},
* {@link SqlConformanceEnum#LENIENT},
* {@link SqlConformanceEnum#MYSQL_5};
+ * {@link SqlConformanceEnum#PRESTO};
Review comment:
did you notice that the items are alphabetic and end in commas except the last?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r392543253
##########
File path: core/src/test/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java
##########
@@ -155,6 +155,8 @@ public MockCatalogReaderSimple(RelDataTypeFactory typeFactory,
deptNestedTable.addColumn("NAME", fixture.varchar10Type);
deptNestedTable.addColumn("SKILL", fixture.skillRecordType);
deptNestedTable.addColumn("EMPLOYEES", fixture.empListType);
+ deptNestedTable.addColumn("ADMINS", fixture.varchar5ArrayType);
+ deptNestedTable.addColumn("OFFICES", fixture.rectilinearPeekCoordMultisetType);
Review comment:
OK. I will add a new table and add above columns in it and use that for tests.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404205509
##########
File path: core/src/main/java/org/apache/calcite/rel/core/Uncollect.java
##########
@@ -46,21 +46,26 @@
public class Uncollect extends SingleRel {
public final boolean withOrdinality;
+ // To alias the items in Uncollect list, e.g., a, b, c in UNNEST(a, b, c),
+ // instead of fields within a struct field
+ private final List<String> fieldAliases;
+
//~ Constructors -----------------------------------------------------------
@Deprecated // to be removed before 2.0
public Uncollect(RelOptCluster cluster, RelTraitSet traitSet,
RelNode child) {
- this(cluster, traitSet, child, false);
+ this(cluster, traitSet, child, false, null);
}
/** Creates an Uncollect.
*
* <p>Use {@link #create} unless you know what you're doing. */
public Uncollect(RelOptCluster cluster, RelTraitSet traitSet, RelNode input,
- boolean withOrdinality) {
+ boolean withOrdinality, List<String> fieldAliases) {
super(cluster, traitSet, input);
this.withOrdinality = withOrdinality;
+ this.fieldAliases = fieldAliases;
Review comment:
you should copy `fieldAliases` into an immutable list.
Do you think the logic would be more uniform if `fieldAliases` were empty, not null, in the usual case? I'm a big fan of empty lists.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404533683
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java
##########
@@ -73,7 +73,11 @@
/** Conformance value that instructs Calcite to use SQL semantics
* consistent with Microsoft SQL Server version 2008. */
- SQL_SERVER_2008;
+ SQL_SERVER_2008,
+
+ /** Conformance value that instructs Calcite to use SQL semantics
+ * consistent with Presto. */
+ PRESTO;
Review comment:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r392047695
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
I know you only to support `SqlCallBinding` for unnest alias columns, but the logic seems weird, can you change
`allowFlattenStruct` => `allowAliasUnnestColumns` ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r380372342
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
@danny0405 Took a second thought on the function name, the result of the function is actually whether **disallow flatten struct in UNNEST**. So maybe disallowFlattenStructInUnnest? Let me know. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r380292169
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/DelegatingScope.java
##########
@@ -298,7 +298,11 @@ public SqlQualified fullyQualify(SqlIdentifier identifier) {
for (; i > 0; i--) {
final SqlIdentifier prefix = identifier.getComponent(0, i);
resolved.clear();
- resolve(prefix.names, nameMatcher, false, resolved);
+ resolve(
+ prefix.names,
+ nameMatcher,
+ validator.getConformance().allowAliasUnnestArrayColumns(),
+ resolved);
Review comment:
Sorry a little confused here. Here I am directly using conformance as the flag. I feel like it's more concise than an explicit flag like
SqlValidatorImpl.java::
boolean allowAliasUnnestArrays = validator.getConformance().allowAliasUnnestArrayColumns;
DelegatingScope.setAllowAliasUnnestArrays(allowAliasUnnestArrays);
DeletatingScope.java::
Just replace validator.getConformance().allowAliasUnnestArrayColumns with the explicit flag.
And it seems using conformance in scope happens before. e.g. OrderByScope.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r392046603
##########
File path: core/src/test/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java
##########
@@ -155,6 +155,8 @@ public MockCatalogReaderSimple(RelDataTypeFactory typeFactory,
deptNestedTable.addColumn("NAME", fixture.varchar10Type);
deptNestedTable.addColumn("SKILL", fixture.skillRecordType);
deptNestedTable.addColumn("EMPLOYEES", fixture.empListType);
+ deptNestedTable.addColumn("ADMINS", fixture.varchar5ArrayType);
+ deptNestedTable.addColumn("OFFICES", fixture.rectilinearPeekCoordMultisetType);
Review comment:
Do not modify existing table/schema to avoid introducing unnecessary change of plans.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1811: [CALCITE-3789]
Support validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-596969698
Reviewing now, may take some time ~
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390167011
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlDialectFactoryImpl.java
##########
@@ -144,7 +145,8 @@ public SqlDialect create(DatabaseMetaData databaseMetaData) {
} else if (databaseProductName.startsWith("HP Neoview")) {
return new NeoviewSqlDialect(c);
} else if (upperProductName.contains("POSTGRE")) {
- return new PostgresqlSqlDialect(c);
+ return new PostgresqlSqlDialect(
+ c.withDataTypeSystem(PostgresqlSqlDialect.POSTGRESQL_TYPE_SYSTEM));
Review comment:
Also a unnecessary change.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404210933
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java
##########
@@ -330,10 +341,19 @@ public boolean allowExtendedTrim() {
case STRICT_92:
case STRICT_99:
case STRICT_2003:
+ case PRESTO:
return false;
default:
return true;
}
}
+ public boolean allowAliasUnnestColumns() {
Review comment:
need space before this function, no space after.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404533701
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java
##########
@@ -431,7 +438,22 @@
* {@link SqlConformanceEnum#STRICT_2003},
* {@link SqlConformanceEnum#ORACLE_10},
* {@link SqlConformanceEnum#ORACLE_12};
+ * {@link SqlConformanceEnum#PRESTO};
* true otherwise.
*/
boolean allowQualifyingCommonColumn();
+
+ /**
+ * Whether directly alias array columns in UNNEST.
+ *
+ * <p>E.g. in UNNEST(a_array, b_array) AS T(a, b),
+ * a and b will be aliases of a_array and b_array
+ * respectively
Review comment:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on issue #1811: [CALCITE-3789] Support
validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-598992058
> > > We should also add a test case for at least one SqlToRelConverterTest.
> >
> >
> > Will do it in the future if needed. I am still unclear that how this should be represented as RelNode. (discussed in https://issues.apache.org/jira/browse/CALCITE-3787)
> > Is it OK for you?
> > EDIT: Some other reasons:
> >
> > 1. We still plan to communicate with Presto using SQL.
> > 2. RelNode -> SqlNode is not quite mature I believe, but I may be wrong there.
> > 3. The optimization enabled by RelNode is helpful. But not a high-pri since Presto will also optimize the internal execution plan.
>
> Usually we have 2 kinds of use cases for Calcite:
>
> * Generates RelNode/RexNode to adapter other downstream planners
> * Generates RelNode/RexNode and unparse it as special SQL dialect to interpretation through JDBC.
>
> Either way we should generates a working plan, that is why I suggest to add SqlToRel test. For more general, we should add operator test and SQL ITCase if we change an operator semantics. But for you case, I think a SqlToRelConverter test is okey.
OK. I could do that for something like unnest(employees) as t(e), but I am not sure what's a valid plan for unnest(employees, admins) as t(e, a). Can I just add one test case for something like unnest(employees) as t(e)? Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404537059
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2219,6 +2207,42 @@ protected void convertFrom(
}
}
+ private List<String> getFieldAliases(SqlCall asCall) {
Review comment:
Hmmm, this makes sense. I will give it more exploration and get back here.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390748598
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlAbstractConformance.java
##########
@@ -111,4 +111,7 @@ public boolean allowQualifyingCommonColumn() {
return SqlConformanceEnum.DEFAULT.allowQualifyingCommonColumn();
}
+ public boolean allowAliasUnnestArrayColumns() {
+ return SqlConformanceEnum.DEFAULT.allowAliasUnnestArrayColumns();
+ }
Review comment:
Could you elaborate here?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390166734
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##########
@@ -451,12 +449,9 @@ public boolean addRule(RelOptRule rule) {
assert added;
final String ruleName = rule.toString();
- if (ruleNames.put(ruleName, rule.getClass())) {
- Set<Class> x = ruleNames.get(ruleName);
- if (x.size() > 1) {
- throw new RuntimeException("Rule description '" + ruleName
- + "' is not unique; classes: " + x);
- }
+ if (ruleNames.put(ruleName, rule) != null) {
+ throw new RuntimeException("Rule description '" + ruleName
Review comment:
Did you merge the master code instead of rebase ? The change seems not that necessary.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r403503969
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2047,8 +2047,15 @@ protected void convertFrom(
case AS:
call = (SqlCall) from;
- convertFrom(bb, call.operand(0));
- if (call.operandCount() > 2
+ SqlNode firstOperand = call.operand(0);
+
+ if (firstOperand.getKind() == SqlKind.UNNEST) {
+ convertUnnest(bb, (SqlCall) firstOperand, getFieldAliases(call));
+ } else {
+ convertFrom(bb, firstOperand);
+ }
Review comment:
Yea, I considered it. For example, for query
{code}
select d.deptno, e, k.empno\n"
+ "from dept_nested_expanded as d CROSS JOIN\n"
+ " UNNEST(d.admins, d.employees) as t(e, k)
{code}
After validation, it will become
{code}
select d.deptno, t.e, t.k.empno\n"
+ "from dept_nested_expanded as d CROSS JOIN\n"
+ " UNNEST(d.admins, d.employees) as t(e, k)
{code}
The failure comes from the Project. Since the rowtype of Uncollect is already flattened, so Calcite could not find the path t.e and t.k.empo from the input, which is the Uncollect RelNode.
So my change will keep them nested, as in SqlUnnestOperator, and I think it's also aligned with the expected behavior, which is unnesting array but not unbox the component struct.
Could you elaborate on your thoughts on AliasNamespace? And if possible, could you provide an ideal logical plan looks like? Thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404533016
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java
##########
@@ -88,6 +88,7 @@
* {@link SqlConformanceEnum#BABEL},
* {@link SqlConformanceEnum#LENIENT},
* {@link SqlConformanceEnum#MYSQL_5};
+ * {@link SqlConformanceEnum#PRESTO};
Review comment:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r391384068
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/DelegatingScope.java
##########
@@ -298,7 +298,11 @@ public SqlQualified fullyQualify(SqlIdentifier identifier) {
for (; i > 0; i--) {
final SqlIdentifier prefix = identifier.getComponent(0, i);
resolved.clear();
- resolve(prefix.names, nameMatcher, false, resolved);
+ resolve(
+ prefix.names,
+ nameMatcher,
+ validator.getConformance().allowAliasUnnestArrayColumns(),
+ resolved);
Review comment:
make sense. Will do
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r403815168
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2047,8 +2047,15 @@ protected void convertFrom(
case AS:
call = (SqlCall) from;
- convertFrom(bb, call.operand(0));
- if (call.operandCount() > 2
+ SqlNode firstOperand = call.operand(0);
+
+ if (firstOperand.getKind() == SqlKind.UNNEST) {
+ convertUnnest(bb, (SqlCall) firstOperand, getFieldAliases(call));
+ } else {
+ convertFrom(bb, firstOperand);
+ }
Review comment:
@danny0405 I don't quite get your point. If you are talking about the decorrelate, I will fix it. But this part of change is not about decorrelate. Could you elaborate more on how does your fix work in more detail? If it's a fundamental change, could you please post it in the ticket too? I will also describe my plan there. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r379968866
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/DelegatingScope.java
##########
@@ -298,7 +298,11 @@ public SqlQualified fullyQualify(SqlIdentifier identifier) {
for (; i > 0; i--) {
final SqlIdentifier prefix = identifier.getComponent(0, i);
resolved.clear();
- resolve(prefix.names, nameMatcher, false, resolved);
+ resolve(
+ prefix.names,
+ nameMatcher,
+ validator.getConformance().allowAliasUnnestArrayColumns(),
+ resolved);
Review comment:
The code may works for your case, but you have changed a common sql identifier resolving logic with a special conformance, which seems not right way to go ~
The conformance switch logic should always happen in SqlValidator, we can pass along some switch flags into the scope if necessary.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390741417
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/DelegatingScope.java
##########
@@ -298,7 +298,11 @@ public SqlQualified fullyQualify(SqlIdentifier identifier) {
for (; i > 0; i--) {
final SqlIdentifier prefix = identifier.getComponent(0, i);
resolved.clear();
- resolve(prefix.names, nameMatcher, false, resolved);
+ resolve(
+ prefix.names,
+ nameMatcher,
+ validator.getConformance().allowAliasUnnestArrayColumns(),
+ resolved);
Review comment:
The current fix seems not right, the nested field can be seen if we set up the record type to `StructKind.PEEK_FIELDS`, for the test cases you gave:
```java
sql("select e.ENAME\n"
+ "from dept_nested as d CROSS JOIN\n"
+ " UNNEST(d.employees) as t(e)")
.withConformance(SqlConformanceEnum.PRESTO).columnType("VARCHAR(10) NOT NULL");
```
There `t(e)` table has record type with `e` as field, the `e` can be seen and if you change the nested employee type with code snippet:
```java
final RelDataType empRecordType = typeFactory.builder()
.add("EMPNO", intType)
.add("ENAME", varchar10Type)
.add("DETAIL", typeFactory.builder()
.add("SKILLS", array(skillRecordType)).build())
.kind(StructKind.PEEK_FIELDS) // here is the line you can tweak
.build();
```
So, in total, this line change is not necessary, the field access should not be controlled by the `SqlConformance`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1811: [CALCITE-3789]
Support validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-590772740
I'm planning to review again this weekend, very busy with the release work of Calcite 1.22 and also my own compony work, sorry for that, Just like Julian said, add a new SqlConformance is really a big change, for "big" i mean, we should keep all the changes correct.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r405954347
##########
File path: core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
##########
@@ -7540,6 +7540,47 @@ public void _testGroupExpressionEquivalenceParams() {
.fails("Column 'C1' not found in any table");
}
+ /**
Review comment:
Remove duplicate-purposed tests, add comments, and add negative cases. Did not break up method since it seems to be not the convention in this test class. And also debating on how to name those tests, whether testAliasUnneestMultipleArrays_forXXX or testAliasUnneestMultipleArraysForXXX
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404537134
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2219,6 +2207,42 @@ protected void convertFrom(
}
}
+ private List<String> getFieldAliases(SqlCall asCall) {
+ if (!validator.getConformance().allowAliasUnnestColumns()) {
+ return null;
+ }
+
+ List<String> columnAliases = new ArrayList<>();
+ for (SqlNode sqlNode : Util.skip(asCall.getOperandList(), 2)) {
+ columnAliases.add(((SqlIdentifier) sqlNode).getSimple());
+ }
+ return columnAliases;
+ }
+
+ private void convertUnnest(Blackboard bb, SqlCall from, List<String> fieldAliases) {
+ SqlCall call;
+ call = from;
Review comment:
This is my bad. Done.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r403805544
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2047,8 +2047,15 @@ protected void convertFrom(
case AS:
call = (SqlCall) from;
- convertFrom(bb, call.operand(0));
- if (call.operandCount() > 2
+ SqlNode firstOperand = call.operand(0);
+
+ if (firstOperand.getKind() == SqlKind.UNNEST) {
+ convertUnnest(bb, (SqlCall) firstOperand, getFieldAliases(call));
+ } else {
+ convertFrom(bb, firstOperand);
+ }
Review comment:
For type flattern, fix the `RelStructuredTypeFlattener` for the operator you care about instead of the `SqlToRelConverter`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390763211
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlAbstractConformance.java
##########
@@ -111,4 +111,7 @@ public boolean allowQualifyingCommonColumn() {
return SqlConformanceEnum.DEFAULT.allowQualifyingCommonColumn();
}
+ public boolean allowAliasUnnestArrayColumns() {
+ return SqlConformanceEnum.DEFAULT.allowAliasUnnestArrayColumns();
+ }
Review comment:
Just `allowAliasUnnestColumns` seems better, no need to specify the operand type, i think.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404219456
##########
File path: core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
##########
@@ -7540,6 +7540,47 @@ public void _testGroupExpressionEquivalenceParams() {
.fails("Column 'C1' not found in any table");
}
+ /**
Review comment:
We need some negative tests, to prove that UNNEST is illegal in non-Presto conformance.
Consider splitting this method up, and adding comments. It's not clear what each test case is testing.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404211535
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java
##########
@@ -431,7 +438,22 @@
* {@link SqlConformanceEnum#STRICT_2003},
* {@link SqlConformanceEnum#ORACLE_10},
* {@link SqlConformanceEnum#ORACLE_12};
+ * {@link SqlConformanceEnum#PRESTO};
* true otherwise.
*/
boolean allowQualifyingCommonColumn();
+
+ /**
+ * Whether directly alias array columns in UNNEST.
+ *
+ * <p>E.g. in UNNEST(a_array, b_array) AS T(a, b),
+ * a and b will be aliases of a_array and b_array
+ * respectively
Review comment:
What is the most logical place for this function inside SqlConformance? I bet it's not at the very end.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on issue #1811: [CALCITE-3789] Support
validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-605682367
> > > We should also add a test case for at least one SqlToRelConverterTest.
> >
> >
> > Will do it in the future if needed. I am still unclear that how this should be represented as RelNode. (discussed in https://issues.apache.org/jira/browse/CALCITE-3787)
> > Is it OK for you?
> > EDIT: Some other reasons:
> >
> > 1. We still plan to communicate with Presto using SQL.
> > 2. RelNode -> SqlNode is not quite mature I believe, but I may be wrong there.
> > 3. The optimization enabled by RelNode is helpful. But not a high-pri since Presto will also optimize the internal execution plan.
>
> Usually we have 2 kinds of use cases for Calcite:
>
> * Generates RelNode/RexNode to adapter other downstream planners
> * Generates RelNode/RexNode and unparse it as special SQL dialect to interpretation through JDBC.
>
> Either way we should generates a working plan, that is why I suggest to add SqlToRel test. For more general, we should add operator test and SQL ITCase if we change an operator semantics. But for you case, I think a SqlToRelConverter test is okey.
I have added tests for it. But it needs changes on SqlToRelConverter.
The change is because usually for STRUCT, Uncollect will flatten the type but for this feature, it should not. This is also aligned with the registered type during validation.
I am not sure whether this should be the best way to implement it. Let me know. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r384145427
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java
##########
@@ -73,7 +73,9 @@
/** Conformance value that instructs Calcite to use SQL semantics
* consistent with Microsoft SQL Server version 2008. */
- SQL_SERVER_2008;
+ SQL_SERVER_2008,
+
+ PRESTO;
Review comment:
I'll add further comments to https://issues.apache.org/jira/browse/CALCITE-3789.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404209579
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java
##########
@@ -73,7 +73,11 @@
/** Conformance value that instructs Calcite to use SQL semantics
* consistent with Microsoft SQL Server version 2008. */
- SQL_SERVER_2008;
+ SQL_SERVER_2008,
+
+ /** Conformance value that instructs Calcite to use SQL semantics
+ * consistent with Presto. */
+ PRESTO;
Review comment:
alphabetical
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r403815168
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2047,8 +2047,15 @@ protected void convertFrom(
case AS:
call = (SqlCall) from;
- convertFrom(bb, call.operand(0));
- if (call.operandCount() > 2
+ SqlNode firstOperand = call.operand(0);
+
+ if (firstOperand.getKind() == SqlKind.UNNEST) {
+ convertUnnest(bb, (SqlCall) firstOperand, getFieldAliases(call));
+ } else {
+ convertFrom(bb, firstOperand);
+ }
Review comment:
@danny0405 I don't quite your point. If you are talking about the decorrelate, I will fix it. But this part of change is not about decorrelate. Could you elaborate more on how does your fix work in more detail? If it's a fundamental change, could you please post it in the ticket too? I will also describe my plan there. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r399841003
##########
File path: core/src/main/java/org/apache/calcite/rel/core/Uncollect.java
##########
@@ -130,25 +139,32 @@ public static RelDataType deriveUncollectRowType(RelNode rel,
// Component type is unknown to Uncollect, build a row type with input column name
// and Any type.
return builder
- .add(fields.get(0).getName(), SqlTypeName.ANY)
+ .add(requireAlias ? fieldAliases.get(0) : fields.get(0).getName(), SqlTypeName.ANY)
.nullable(true)
.build();
}
+
+ int aliasIndex = 0;
for (RelDataTypeField field : fields) {
if (field.getType() instanceof MapSqlType) {
builder.add(SqlUnnestOperator.MAP_KEY_COLUMN_NAME, field.getType().getKeyType());
builder.add(SqlUnnestOperator.MAP_VALUE_COLUMN_NAME, field.getType().getValueType());
} else {
Review comment:
I am also not sure what will be the best way to handle MAP here.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390753540
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlAbstractConformance.java
##########
@@ -111,4 +111,7 @@ public boolean allowQualifyingCommonColumn() {
return SqlConformanceEnum.DEFAULT.allowQualifyingCommonColumn();
}
+ public boolean allowAliasUnnestArrayColumns() {
+ return SqlConformanceEnum.DEFAULT.allowAliasUnnestArrayColumns();
+ }
Review comment:
I mean you `unnest` support `multiset` type which is also supported in your code. But the method name here does not represent that.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390620129
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlDialectFactoryImpl.java
##########
@@ -144,7 +145,8 @@ public SqlDialect create(DatabaseMetaData databaseMetaData) {
} else if (databaseProductName.startsWith("HP Neoview")) {
return new NeoviewSqlDialect(c);
} else if (upperProductName.contains("POSTGRE")) {
- return new PostgresqlSqlDialect(c);
+ return new PostgresqlSqlDialect(
+ c.withDataTypeSystem(PostgresqlSqlDialect.POSTGRESQL_TYPE_SYSTEM));
Review comment:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390761482
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlAbstractConformance.java
##########
@@ -111,4 +111,7 @@ public boolean allowQualifyingCommonColumn() {
return SqlConformanceEnum.DEFAULT.allowQualifyingCommonColumn();
}
+ public boolean allowAliasUnnestArrayColumns() {
+ return SqlConformanceEnum.DEFAULT.allowAliasUnnestArrayColumns();
+ }
Review comment:
Got it. Do you have a better name in mind? I picked array because it seems to be a little more common (at least for me). But I am OK with other names. Let me know.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390741950
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
I mean `RexCallBinding` comes from building the call from `RelBuilder`, but why we return true for that, confusing for me.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r392543166
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
Good idea. Will do
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r380288896
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
> Another class type for operator binding is `RexCallBinding`, but why we return true for that ?
This is the original behavior. Actually there are tons of SqlOperatorBinding implementations. As the original behavior, UNNEST will ALWAYS flatten the struct. The logic here is that we do NOT allow flatten struct ONLY for SqlCallBinding && conformance.allowAliasUnnestArrayColumns()
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390167011
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlDialectFactoryImpl.java
##########
@@ -144,7 +145,8 @@ public SqlDialect create(DatabaseMetaData databaseMetaData) {
} else if (databaseProductName.startsWith("HP Neoview")) {
return new NeoviewSqlDialect(c);
} else if (upperProductName.contains("POSTGRE")) {
- return new PostgresqlSqlDialect(c);
+ return new PostgresqlSqlDialect(
+ c.withDataTypeSystem(PostgresqlSqlDialect.POSTGRESQL_TYPE_SYSTEM));
Review comment:
Also an unnecessary change.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 edited a comment on issue #1811: [CALCITE-3789]
Support validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
danny0405 edited a comment on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-590772740
I'm planning to review again this weekend, very busy with the release work of Calcite 1.22 and also my own company work, sorry for that, Just like Julian said, add a new SqlConformance is really a big change, for "big" i mean, we should keep all the changes correct.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym edited a comment on issue #1811: [CALCITE-3789]
Support validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
my7ym edited a comment on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-597447584
> We should also add a test case for at least one SqlToRelConverterTest.
Will do it in the future if needed. I am still unclear that how this should be represented as RelNode. (discussed in https://issues.apache.org/jira/browse/CALCITE-3787)
Is it OK for you?
EDIT: Some other reasons:
1. We still plan to communicate with Presto using SQL.
2. RelNode -> SqlNode is not quite mature I believe, but I may be wrong there.
3. The optimization enabled by RelNode is helpful. But not a high-pri since Presto will also optimize the internal execution plan.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390741619
##########
File path: core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
##########
@@ -7540,6 +7540,42 @@ public void _testGroupExpressionEquivalenceParams() {
.fails("Column 'C1' not found in any table");
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-3789">[CALCITE-3789]
+ * Support validation of UNNEST multiple array columns like Presto</a>.
+ */
+ @Test public void testAliasUnnestMultipleArrays() {
+ sql("select A, varType "
+ + "from COMPLEXTYPES.CTC_T1 "
+ + "CROSS JOIN UNNEST(varchar5ArrayType) as t(varType)")
+ .withExtendedCatalog()
+ .withCaseSensitive(false)
+ .withConformance(SqlConformanceEnum.PRESTO).ok();
+ sql("select A, varType, intType "
+ + "from COMPLEXTYPES.CTC_T1 "
+ + "CROSS JOIN UNNEST(varchar5ArrayType, intArrayType) as t(varType, intType)")
+ .withExtendedCatalog()
+ .withCaseSensitive(false)
+ .withConformance(SqlConformanceEnum.PRESTO).ok();
+ sql("select e.ENAME\n"
+ + "from dept_nested as d CROSS JOIN\n"
+ + " UNNEST(d.employees) as t(e)")
+ .withConformance(SqlConformanceEnum.PRESTO).columnType("VARCHAR(10) NOT NULL");
+ sql("select d.deptno, e.detail, k.empno\n"
+ + "from dept_nested as d CROSS JOIN\n"
+ + " UNNEST(d.employees, d.employees) as t(e, k)")
Review comment:
You should add a new nested `StructKind.PEEK_FIELDS` type and write test with that.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404215706
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2219,6 +2207,42 @@ protected void convertFrom(
}
}
+ private List<String> getFieldAliases(SqlCall asCall) {
+ if (!validator.getConformance().allowAliasUnnestColumns()) {
+ return null;
+ }
+
+ List<String> columnAliases = new ArrayList<>();
+ for (SqlNode sqlNode : Util.skip(asCall.getOperandList(), 2)) {
+ columnAliases.add(((SqlIdentifier) sqlNode).getSimple());
+ }
+ return columnAliases;
+ }
+
+ private void convertUnnest(Blackboard bb, SqlCall from, List<String> fieldAliases) {
+ SqlCall call;
+ call = from;
Review comment:
assign on one line and make final? or eliminate a variable?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390741479
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlAbstractConformance.java
##########
@@ -111,4 +111,7 @@ public boolean allowQualifyingCommonColumn() {
return SqlConformanceEnum.DEFAULT.allowQualifyingCommonColumn();
}
+ public boolean allowAliasUnnestArrayColumns() {
+ return SqlConformanceEnum.DEFAULT.allowAliasUnnestArrayColumns();
+ }
Review comment:
How about the `MultiSet` type.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404209413
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java
##########
@@ -431,7 +438,22 @@
* {@link SqlConformanceEnum#STRICT_2003},
* {@link SqlConformanceEnum#ORACLE_10},
* {@link SqlConformanceEnum#ORACLE_12};
+ * {@link SqlConformanceEnum#PRESTO};
* true otherwise.
*/
boolean allowQualifyingCommonColumn();
+
+ /**
+ * Whether directly alias array columns in UNNEST.
+ *
+ * <p>E.g. in UNNEST(a_array, b_array) AS T(a, b),
+ * a and b will be aliases of a_array and b_array
+ * respectively
Review comment:
What will be the behavior of that expression if `allowAliasUnnestColumns` is false?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404532978
##########
File path: core/src/main/java/org/apache/calcite/rel/core/Uncollect.java
##########
@@ -46,21 +46,26 @@
public class Uncollect extends SingleRel {
public final boolean withOrdinality;
+ // To alias the items in Uncollect list, e.g., a, b, c in UNNEST(a, b, c),
+ // instead of fields within a struct field
+ private final List<String> fieldAliases;
+
//~ Constructors -----------------------------------------------------------
@Deprecated // to be removed before 2.0
public Uncollect(RelOptCluster cluster, RelTraitSet traitSet,
RelNode child) {
- this(cluster, traitSet, child, false);
+ this(cluster, traitSet, child, false, null);
}
/** Creates an Uncollect.
*
* <p>Use {@link #create} unless you know what you're doing. */
public Uncollect(RelOptCluster cluster, RelTraitSet traitSet, RelNode input,
- boolean withOrdinality) {
+ boolean withOrdinality, List<String> fieldAliases) {
super(cluster, traitSet, input);
this.withOrdinality = withOrdinality;
+ this.fieldAliases = fieldAliases;
Review comment:
Done. Usually I think null is a separate state to flag something other than empty. But here they seem to be the same and we have extra bonus to avoid NPE. Good call.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1811: [CALCITE-3789]
Support validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-598983239
> > We should also add a test case for at least one SqlToRelConverterTest.
>
> Will do it in the future if needed. I am still unclear that how this should be represented as RelNode. (discussed in https://issues.apache.org/jira/browse/CALCITE-3787)
>
> Is it OK for you?
>
> EDIT: Some other reasons:
>
> 1. We still plan to communicate with Presto using SQL.
> 2. RelNode -> SqlNode is not quite mature I believe, but I may be wrong there.
> 3. The optimization enabled by RelNode is helpful. But not a high-pri since Presto will also optimize the internal execution plan.
Usually we have 2 kinds of use cases for Calcite:
- Generates RelNode/RexNode to adapter other downstream planners
- Generates RelNode/RexNode and unparse it as special SQL dialect to interpretation through JDBC.
Either way we should generates a working plan, that is why I suggest to add SqlToRel test. For more general, we should add operator test and SQL ITCase if we change an operator semantics. But for you case, I think a SqlToRelConverter test is okey.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1811: [CALCITE-3789]
Support validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-597440250
We should also add a test case for at least one SqlToRelConverterTest.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404536366
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2047,8 +2047,15 @@ protected void convertFrom(
case AS:
call = (SqlCall) from;
- convertFrom(bb, call.operand(0));
- if (call.operandCount() > 2
+ SqlNode firstOperand = call.operand(0);
+
+ if (firstOperand.getKind() == SqlKind.UNNEST) {
+ convertUnnest(bb, (SqlCall) firstOperand, getFieldAliases(call));
+ } else {
+ convertFrom(bb, firstOperand);
+ }
Review comment:
I am still a little confused. Will try it and get back here later. Let me know if you folks have more concrete instructions on this. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on issue #1811: [CALCITE-3789] Support
validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-605682713
> We should also add a test case for at least one SqlToRelConverterTest.
Done
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390620022
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##########
@@ -451,12 +449,9 @@ public boolean addRule(RelOptRule rule) {
assert added;
final String ruleName = rule.toString();
- if (ruleNames.put(ruleName, rule.getClass())) {
- Set<Class> x = ruleNames.get(ruleName);
- if (x.size() > 1) {
- throw new RuntimeException("Rule description '" + ruleName
- + "' is not unique; classes: " + x);
- }
+ if (ruleNames.put(ruleName, rule) != null) {
+ throw new RuntimeException("Rule description '" + ruleName
Review comment:
@danny0405 Done
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404212431
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2047,8 +2047,15 @@ protected void convertFrom(
case AS:
call = (SqlCall) from;
- convertFrom(bb, call.operand(0));
- if (call.operandCount() > 2
+ SqlNode firstOperand = call.operand(0);
+
+ if (firstOperand.getKind() == SqlKind.UNNEST) {
+ convertUnnest(bb, (SqlCall) firstOperand, getFieldAliases(call));
+ } else {
+ convertFrom(bb, firstOperand);
+ }
Review comment:
I agree, AliasNamespace sounds like a good direction to explore.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on issue #1811: [CALCITE-3789] Support
validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-588041793
@danny0405 Just a kindly reminder, could you please take another look when available? Thanks a lot!
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404217025
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2219,6 +2207,42 @@ protected void convertFrom(
}
}
+ private List<String> getFieldAliases(SqlCall asCall) {
+ if (!validator.getConformance().allowAliasUnnestColumns()) {
+ return null;
+ }
+
+ List<String> columnAliases = new ArrayList<>();
+ for (SqlNode sqlNode : Util.skip(asCall.getOperandList(), 2)) {
+ columnAliases.add(((SqlIdentifier) sqlNode).getSimple());
+ }
+ return columnAliases;
+ }
+
+ private void convertUnnest(Blackboard bb, SqlCall from, List<String> fieldAliases) {
+ SqlCall call;
+ call = from;
+ final List<SqlNode> nodes = call.getOperandList();
+ final SqlUnnestOperator operator = (SqlUnnestOperator) call.getOperator();
+ for (SqlNode node : nodes) {
+ replaceSubQueries(bb, node, RelOptUtil.Logic.TRUE_FALSE_UNKNOWN);
+ }
+ final List<RexNode> exprs = new ArrayList<>();
+ final List<String> fieldNames = new ArrayList<>();
Review comment:
you don't need two separate arrays. `RelBuilder.project` will apply aliases if you have used `RelBuilder.alias` to wrap expressions in 'as'. Use `project` rather than `projectNamed`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r392535735
##########
File path: core/src/test/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java
##########
@@ -155,6 +155,8 @@ public MockCatalogReaderSimple(RelDataTypeFactory typeFactory,
deptNestedTable.addColumn("NAME", fixture.varchar10Type);
deptNestedTable.addColumn("SKILL", fixture.skillRecordType);
deptNestedTable.addColumn("EMPLOYEES", fixture.empListType);
+ deptNestedTable.addColumn("ADMINS", fixture.varchar5ArrayType);
+ deptNestedTable.addColumn("OFFICES", fixture.rectilinearPeekCoordMultisetType);
Review comment:
-1 because we should never break existing test because of not relevant change.
I don’t care if you reuse the test code base but at least let’s not break other tests.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on issue #1811: [CALCITE-3789] Support
validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-590705273
@danny0405 Could you help take another look at this PR? Thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404215075
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2219,6 +2207,42 @@ protected void convertFrom(
}
}
+ private List<String> getFieldAliases(SqlCall asCall) {
Review comment:
This method needs javadoc
Is returning null the best thing? If it's illegal to call the method, you should throw. I hate nullable returns.
Conformance usually applies during validation, not during sql-to-rel. You know validation has succeeded at this point. If only one thing is possible, you should just do it.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404217847
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2219,6 +2207,42 @@ protected void convertFrom(
}
}
+ private List<String> getFieldAliases(SqlCall asCall) {
+ if (!validator.getConformance().allowAliasUnnestColumns()) {
+ return null;
+ }
+
+ List<String> columnAliases = new ArrayList<>();
+ for (SqlNode sqlNode : Util.skip(asCall.getOperandList(), 2)) {
+ columnAliases.add(((SqlIdentifier) sqlNode).getSimple());
+ }
+ return columnAliases;
+ }
+
+ private void convertUnnest(Blackboard bb, SqlCall from, List<String> fieldAliases) {
+ SqlCall call;
+ call = from;
+ final List<SqlNode> nodes = call.getOperandList();
+ final SqlUnnestOperator operator = (SqlUnnestOperator) call.getOperator();
+ for (SqlNode node : nodes) {
+ replaceSubQueries(bb, node, RelOptUtil.Logic.TRUE_FALSE_UNKNOWN);
+ }
+ final List<RexNode> exprs = new ArrayList<>();
+ final List<String> fieldNames = new ArrayList<>();
+ for (Ord<SqlNode> node : Ord.zip(nodes)) {
+ exprs.add(bb.convertExpression(node.e));
+ fieldNames.add(validator.deriveAlias(node.e, node.i));
+ }
+ RelNode child =
+ (null != bb.root) ? bb.root : LogicalValues.createOneRow(cluster);
+ relBuilder.push(child).projectNamed(exprs, fieldNames, false);
+
+ Uncollect uncollect =
Review comment:
it's about time we added `RelBuilder.uncollect`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r392326029
##########
File path: core/src/test/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java
##########
@@ -155,6 +155,8 @@ public MockCatalogReaderSimple(RelDataTypeFactory typeFactory,
deptNestedTable.addColumn("NAME", fixture.varchar10Type);
deptNestedTable.addColumn("SKILL", fixture.skillRecordType);
deptNestedTable.addColumn("EMPLOYEES", fixture.empListType);
+ deptNestedTable.addColumn("ADMINS", fixture.varchar5ArrayType);
+ deptNestedTable.addColumn("OFFICES", fixture.rectilinearPeekCoordMultisetType);
Review comment:
I am really confused about your review.
What do you mean by
> You should add a new nested StructKind.PEEK_FIELDS type and write test with that.
Right now you again think we should create a new table/schema to avoid it. I guess it's expected for you to know that this table is used in a lot of places. Maybe I am wrong here?
And what's the drawbacks of so? It's test fixtures and should be shared with multiple tests. Do we need to create a new test fixtures for every PR if it changes other test assertions?
If you insist, please think it again and let me know if I create another table with something like
` deptNestedExtended.addColumn("NAME", fixture.varchar10Type);
deptNestedExtended.addColumn("ADMINS", fixture.varchar5ArrayType);
deptNestedExtended.addColumn("EMPLOYEES", fixture.empListType);
deptNestedExtended.addColumn("OFFICES", fixture.rectilinearPeekCoordMultisetType);
`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on issue #1811: [CALCITE-3789] Support
validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-594318631
@danny0405 @julianhyde Just a soft reminder on this PR. No rush. Thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r399838961
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -1304,7 +1304,31 @@ public final Sql sql(String sql) {
final String sql = "select d.deptno, e2.empno\n"
+ "from dept_nested as d,\n"
+ " UNNEST(d.employees) as e2(empno, y, z)";
- sql(sql).with(getExtendedTester()).ok();
+ sql(sql).ok();
+ }
+
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-3789">[CALCITE-3789]
+ * Support validation of UNNEST multiple array columns like Presto</a>.
+ */
+ @Test public void testAliasUnnestArrayPlanWithSingleColumn() {
+ final String sql = "select d.deptno, employee.empno\n"
+ + "from dept_nested_expanded as d,\n"
+ + " UNNEST(d.employees) as t(employee)";
+ sql(sql).conformance(SqlConformanceEnum.PRESTO).decorrelate(false).ok();
Review comment:
The reason of turning off `decorrelate` is because during flattening, the row type of Uncollect is not fully flattened, but the index is calculated on a fully flattened base (RelStructuredTypeFlattener.postFlattenSize). So the type mismatches validated query. Not sure whether it will be a big issue tho.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r392543103
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
That's a good idea.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r390748624
##########
File path: core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
##########
@@ -7540,6 +7540,42 @@ public void _testGroupExpressionEquivalenceParams() {
.fails("Column 'C1' not found in any table");
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-3789">[CALCITE-3789]
+ * Support validation of UNNEST multiple array columns like Presto</a>.
+ */
+ @Test public void testAliasUnnestMultipleArrays() {
+ sql("select A, varType "
+ + "from COMPLEXTYPES.CTC_T1 "
+ + "CROSS JOIN UNNEST(varchar5ArrayType) as t(varType)")
+ .withExtendedCatalog()
+ .withCaseSensitive(false)
+ .withConformance(SqlConformanceEnum.PRESTO).ok();
+ sql("select A, varType, intType "
+ + "from COMPLEXTYPES.CTC_T1 "
+ + "CROSS JOIN UNNEST(varchar5ArrayType, intArrayType) as t(varType, intType)")
+ .withExtendedCatalog()
+ .withCaseSensitive(false)
+ .withConformance(SqlConformanceEnum.PRESTO).ok();
+ sql("select e.ENAME\n"
+ + "from dept_nested as d CROSS JOIN\n"
+ + " UNNEST(d.employees) as t(e)")
+ .withConformance(SqlConformanceEnum.PRESTO).columnType("VARCHAR(10) NOT NULL");
+ sql("select d.deptno, e.detail, k.empno\n"
+ + "from dept_nested as d CROSS JOIN\n"
+ + " UNNEST(d.employees, d.employees) as t(e, k)")
Review comment:
Will do
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404539706
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2219,6 +2207,42 @@ protected void convertFrom(
}
}
+ private List<String> getFieldAliases(SqlCall asCall) {
+ if (!validator.getConformance().allowAliasUnnestColumns()) {
+ return null;
+ }
+
+ List<String> columnAliases = new ArrayList<>();
+ for (SqlNode sqlNode : Util.skip(asCall.getOperandList(), 2)) {
+ columnAliases.add(((SqlIdentifier) sqlNode).getSimple());
+ }
+ return columnAliases;
+ }
+
+ private void convertUnnest(Blackboard bb, SqlCall from, List<String> fieldAliases) {
+ SqlCall call;
+ call = from;
+ final List<SqlNode> nodes = call.getOperandList();
+ final SqlUnnestOperator operator = (SqlUnnestOperator) call.getOperator();
+ for (SqlNode node : nodes) {
+ replaceSubQueries(bb, node, RelOptUtil.Logic.TRUE_FALSE_UNKNOWN);
+ }
+ final List<RexNode> exprs = new ArrayList<>();
+ final List<String> fieldNames = new ArrayList<>();
Review comment:
Done. btw this logic is there already before I extract it as a separate method. I usually avoid to touch unnecessary part as much as possible.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on issue #1811: [CALCITE-3789] Support
validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-591192111
> I'm planning to review again this weekend, very busy with the release work of Calcite 1.22 and also my own company work, sorry for that, Just like Julian said, add a new SqlConformance is really a big change, for "big" i mean, we should keep all the changes correct.
Make sense. Just send a reminder and get a rough timeline. Thanks @danny0405
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404536253
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java
##########
@@ -330,10 +341,19 @@ public boolean allowExtendedTrim() {
case STRICT_92:
case STRICT_99:
case STRICT_2003:
+ case PRESTO:
return false;
default:
return true;
}
}
+ public boolean allowAliasUnnestColumns() {
Review comment:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404537725
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -1304,7 +1304,31 @@ public final Sql sql(String sql) {
final String sql = "select d.deptno, e2.empno\n"
+ "from dept_nested as d,\n"
+ " UNNEST(d.employees) as e2(empno, y, z)";
- sql(sql).with(getExtendedTester()).ok();
+ sql(sql).ok();
+ }
+
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-3789">[CALCITE-3789]
+ * Support validation of UNNEST multiple array columns like Presto</a>.
+ */
+ @Test public void testAliasUnnestArrayPlanWithSingleColumn() {
+ final String sql = "select d.deptno, employee.empno\n"
+ + "from dept_nested_expanded as d,\n"
+ + " UNNEST(d.employees) as t(employee)";
+ sql(sql).conformance(SqlConformanceEnum.PRESTO).decorrelate(false).ok();
Review comment:
Will do
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1811: [CALCITE-3789]
Support validation of UNNEST multiple array columns like Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#issuecomment-596972529
Thanks @my7ym , could you rebase you code to remove the unnecessary change ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r379968141
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
`allowFlattenStruct` -> `allowAliasUnnestArrayColumns` ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r392326029
##########
File path: core/src/test/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java
##########
@@ -155,6 +155,8 @@ public MockCatalogReaderSimple(RelDataTypeFactory typeFactory,
deptNestedTable.addColumn("NAME", fixture.varchar10Type);
deptNestedTable.addColumn("SKILL", fixture.skillRecordType);
deptNestedTable.addColumn("EMPLOYEES", fixture.empListType);
+ deptNestedTable.addColumn("ADMINS", fixture.varchar5ArrayType);
+ deptNestedTable.addColumn("OFFICES", fixture.rectilinearPeekCoordMultisetType);
Review comment:
I am really confused about your review.
What do you mean by
> You should add a new nested StructKind.PEEK_FIELDS type and write test with that.
Right now you again think we should create a new table/schema to avoid it. I guess it's expected for you to know that this table is used in a lot of places. Maybe I am wrong here?
And what's the drawbacks of so? It's test fixtures and should be shared with multiple tests. Do we need to create a new test fixtures for every PR if it changes other test assertions?
If you insist, please think it again and let me know if I create another table with something like
` deptNestedExtended.addColumn("NAME", fixture.varchar10Type);
deptNestedExtended.addColumn("ADMINS", fixture.varchar5ArrayType);
deptNestedExtended.addColumn("EMPLOYEES", fixture.empListType);
deptNestedExtended.addColumn("OFFICES", fixture.rectilinearPeekCoordMultisetType);
`
works here? If so, I will do that. Thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r379968267
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
Another class type for operator binding is `RexCallBinding`, but why we return true for that ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r404546322
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -2219,6 +2207,42 @@ protected void convertFrom(
}
}
+ private List<String> getFieldAliases(SqlCall asCall) {
+ if (!validator.getConformance().allowAliasUnnestColumns()) {
+ return null;
+ }
+
+ List<String> columnAliases = new ArrayList<>();
+ for (SqlNode sqlNode : Util.skip(asCall.getOperandList(), 2)) {
+ columnAliases.add(((SqlIdentifier) sqlNode).getSimple());
+ }
+ return columnAliases;
+ }
+
+ private void convertUnnest(Blackboard bb, SqlCall from, List<String> fieldAliases) {
+ SqlCall call;
+ call = from;
+ final List<SqlNode> nodes = call.getOperandList();
+ final SqlUnnestOperator operator = (SqlUnnestOperator) call.getOperator();
+ for (SqlNode node : nodes) {
+ replaceSubQueries(bb, node, RelOptUtil.Logic.TRUE_FALSE_UNKNOWN);
+ }
+ final List<RexNode> exprs = new ArrayList<>();
+ final List<String> fieldNames = new ArrayList<>();
+ for (Ord<SqlNode> node : Ord.zip(nodes)) {
+ exprs.add(bb.convertExpression(node.e));
+ fieldNames.add(validator.deriveAlias(node.e, node.i));
+ }
+ RelNode child =
+ (null != bb.root) ? bb.root : LogicalValues.createOneRow(cluster);
+ relBuilder.push(child).projectNamed(exprs, fieldNames, false);
+
+ Uncollect uncollect =
Review comment:
Done.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] my7ym commented on a change in pull request #1811:
[CALCITE-3789] Support validation of UNNEST multiple array columns like
Presto
Posted by GitBox <gi...@apache.org>.
my7ym commented on a change in pull request #1811: [CALCITE-3789] Support validation of UNNEST multiple array columns like Presto
URL: https://github.com/apache/calcite/pull/1811#discussion_r391388454
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java
##########
@@ -97,6 +97,17 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+ if (!(operatorBinding instanceof SqlCallBinding)) {
Review comment:
Again, this is original behavior. I feel like for a PR, it could be better to only contain all the relevant changes without changing existing behaviors for irrelevant flows.
There are 8 other subclasses of `SqlOperatorBinding` and I guess several of them should not be here. If you believe this is a serious issue and should be addressed, we should have another refactor for it.
Let me know if you insist on gating the `RexCallBinding`, I will add another commit to do it.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services