twalthr commented on code in PR #24255:
URL: https://github.com/apache/flink/pull/24255#discussion_r1837684705
##########
flink-table/flink-sql-parser/src/main/codegen/templates/Parser.jj:
##########
@@ -4581,15 +4608,24 @@ SqlLiteral DateTimeLiteral() :
}
|
<DATE> { s = span(); } p = SimpleStringLiteral() {
- return SqlParserUtil.parseDateLiteral(p, s.end(this));
+ return SqlLiteral.createUnknown("DATE", p, s.end(this));
+ }
+|
+ <DATETIME> { s = span(); } p = SimpleStringLiteral() {
+ return SqlLiteral.createUnknown("DATETIME", p, s.end(this));
}
|
<TIME> { s = span(); } p = SimpleStringLiteral() {
- return SqlParserUtil.parseTimeLiteral(p, s.end(this));
+ return SqlLiteral.createUnknown("TIME", p, s.end(this));
}
|
+ LOOKAHEAD(2)
<TIMESTAMP> { s = span(); } p = SimpleStringLiteral() {
- return SqlParserUtil.parseTimestampLiteral(p, s.end(this));
+ return SqlLiteral.createUnknown("TIMESTAMP", p, s.end(this));
+ }
+|
+ <TIMESTAMP> { s = span(); } <WITH> <LOCAL> <TIME> <ZONE> p =
SimpleStringLiteral() {
Review Comment:
we should update our docs that `TIMESTAMP WITH LOCAL TIME ZONE ‘1969-07-20
20:17:40’` is supported. Is that the case now? we should add a test case.
##########
flink-table/flink-sql-parser/src/main/codegen/templates/Parser.jj:
##########
@@ -4503,6 +4519,17 @@ SqlNode StringLiteral() :
return SqlStdOperatorTable.LITERAL_CHAIN.createCall(pos2, rands);
}
}
+|
+ <C_STYLE_ESCAPED_STRING_LITERAL>
Review Comment:
update docs:
https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/sql/queries/overview/#syntax
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/validate/FlinkSqlConformance.java:
##########
@@ -166,4 +166,14 @@ public boolean allowQualifyingCommonColumn() {
public SqlLibrary semantics() {
return SqlConformanceEnum.DEFAULT.semantics();
}
+
+ @Override
+ public boolean allowCoercionStringToArray() {
+ return SqlConformanceEnum.DEFAULT.allowCoercionStringToArray();
+ }
+
+ @Override
+ public boolean isValueAllowed() {
+ return SqlConformanceEnum.DEFAULT.isValueAllowed();
Review Comment:
check if false, but it would be fine if it is true
##########
flink-table/flink-table-calcite-bridge/pom.xml:
##########
@@ -45,23 +45,23 @@ under the License.
<version>${calcite.version}</version>
<exclusions>
<!--
- "mvn dependency:tree" as of Calcite 1.32.0:
- [INFO] +-
org.apache.calcite:calcite-core:jar:1.32.0:compile
- [INFO] | +-
org.apache.calcite:calcite-linq4j:jar:1.32.0:compile
+ "mvn dependency:tree" as of Calcite 1.33.0:
+ [INFO] +-
org.apache.calcite:calcite-core:jar:1.33.0:compile
+ [INFO] | +-
org.apache.calcite:calcite-linq4j:jar:1.33.0:compile
[INFO] | +-
org.locationtech.jts:jts-core:jar:1.19.0:compile
- [INFO] | +-
com.fasterxml.jackson.core:jackson-annotations:jar:2.14.3:compile
- [INFO] | +-
org.apache.calcite.avatica:avatica-core:jar:1.22.0:compile
+ [INFO] | +-
com.fasterxml.jackson.core:jackson-annotations:jar:2.15.3:compile
+ [INFO] | +-
org.apache.calcite.avatica:avatica-core:jar:1.23.0:compile
[INFO] | +-
org.apiguardian:apiguardian-api:jar:1.1.2:compile
- [INFO] | +-
com.fasterxml.jackson.core:jackson-core:jar:2.14.3:compile
- [INFO] | +-
com.fasterxml.jackson.core:jackson-databind:jar:2.14.3:compile
+ [INFO] | +-
com.fasterxml.jackson.core:jackson-core:jar:2.15.3:compile
+ [INFO] | +-
com.fasterxml.jackson.core:jackson-databind:jar:2.15.3:compile
[INFO] | +-
com.jayway.jsonpath:json-path:jar:2.7.0:runtime
[INFO] | | \-
net.minidev:json-smart:jar:2.4.7:runtime
[INFO] | | \-
net.minidev:accessors-smart:jar:2.4.7:runtime
[INFO] | | \-
org.ow2.asm:asm:jar:9.1:runtime
[INFO] | +-
commons-codec:commons-codec:jar:1.15:runtime
+ [INFO] | +-
org.apache.commons:commons-math3:jar:3.6.1:runtime
Review Comment:
check if this collides / overlaps with flink-core?
##########
flink-table/flink-table-planner/src/main/java/org/apache/calcite/rex/RexUtil.java:
##########
@@ -0,0 +1,3176 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.rex;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Range;
+import org.apache.calcite.DataContexts;
+import org.apache.calcite.linq4j.function.Predicate1;
+import org.apache.calcite.plan.RelOptPredicateList;
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.rel.RelCollation;
+import org.apache.calcite.rel.RelCollations;
+import org.apache.calcite.rel.RelFieldCollation;
+import org.apache.calcite.rel.core.Filter;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.Project;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeFamily;
+import org.apache.calcite.rel.type.RelDataTypeField;
+import org.apache.calcite.rex.RexTableInputRef.RelTableRef;
+import org.apache.calcite.sql.SqlAggFunction;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.type.SqlTypeFamily;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeUtil;
+import org.apache.calcite.sql.validate.SqlValidatorUtil;
+import org.apache.calcite.util.ControlFlowException;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.Litmus;
+import org.apache.calcite.util.Pair;
+import org.apache.calcite.util.RangeSets;
+import org.apache.calcite.util.Sarg;
+import org.apache.calcite.util.Util;
+import org.apache.calcite.util.mapping.Mappings;
+import org.apiguardian.api.API;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.function.Predicate;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link org.apache.calcite.rex.RexUtil}, the class
was copied over
+ * because of current Calcite way of inferring constants from IS NOT DISTINCT
FROM clashes with
+ * filter push down.
+ *
+ * <p>Lines 397 ~ 399, Use Calcite 1.32.0 behavior for {@link
RexUtil#gatherConstraints(Class,
Review Comment:
investigate this in a follow up issue
##########
flink-table/flink-sql-parser/src/main/codegen/templates/Parser.jj:
##########
@@ -4935,103 +5031,75 @@ SqlIntervalQualifier IntervalQualifierStart() :
}
}
-/**
- * Parses time unit for CEIL and FLOOR functions.
- */
-TimeUnit TimeUnit() :
-{
- final TimeUnit unit;
-}
-{
- LOOKAHEAD(1)
- (
- <MILLISECOND> { return TimeUnit.MILLISECOND; }
- | <SECOND> { return TimeUnit.SECOND; }
- | <MINUTE> { return TimeUnit.MINUTE; }
- | <HOUR> { return TimeUnit.HOUR; }
- | <DAY> { return TimeUnit.DAY; }
- | <DOW> { return TimeUnit.DOW; }
- | <DOY> { return TimeUnit.DOY; }
- | <ISODOW> { return TimeUnit.ISODOW; }
- | <ISOYEAR> { return TimeUnit.ISOYEAR; }
- | <WEEK> { return TimeUnit.WEEK; }
- | <MONTH> { return TimeUnit.MONTH; }
- | <QUARTER> { return TimeUnit.QUARTER; }
- | <YEAR> { return TimeUnit.YEAR; }
- | <EPOCH> { return TimeUnit.EPOCH; }
- | <DECADE> { return TimeUnit.DECADE; }
- | <CENTURY> { return TimeUnit.CENTURY; }
- | <MILLENNIUM> { return TimeUnit.MILLENNIUM; }
- )
-|
- unit = TimeUnitIdentifier() { return unit; }
-}
-
-/**
- * Parses time unit for the EXTRACT function.
- * As for FLOOR and CEIL, but also includes NANOSECOND and MICROSECOND.
+/** Parses a built-in time unit (e.g. "YEAR")
+ * or user-defined time frame (e.g. "MINUTE15")
+ * and in each case returns a {@link SqlIntervalQualifier}.
+ *
+ * <p>The units are used in several functions, incuding CEIL, FLOOR, EXTRACT.
+ * Includes NANOSECOND, MILLISECOND, which were previously allowed in EXTRACT
+ * but not CEIL, FLOOR.
+ *
+ * <p>Includes {@code WEEK} and {@code WEEK(SUNDAY)} through
+ {@code WEEK(SATURDAY)}.
+ *
+ * <p>Does not include SQL_TSI_DAY, SQL_TSI_FRAC_SECOND etc. These will be
+ * parsed as identifiers and can be resolved in the validator if they are
+ * registered as abbreviations in your time frame set.
*/
-TimeUnit TimeUnitForExtract() :
-{
+SqlIntervalQualifier TimeUnitOrName() : {
+ final Span span;
+ final String w;
final TimeUnit unit;
+ final SqlIdentifier unitName;
}
{
- LOOKAHEAD(1)
+ LOOKAHEAD(2)
+ <NANOSECOND> { return new SqlIntervalQualifier(TimeUnit.NANOSECOND, null,
getPos()); }
+| <MICROSECOND> { return new SqlIntervalQualifier(TimeUnit.MICROSECOND,
null, getPos()); }
+| <MILLISECOND> { return new SqlIntervalQualifier(TimeUnit.MILLISECOND,
null, getPos()); }
+| <SECOND> { return new SqlIntervalQualifier(TimeUnit.SECOND, null,
getPos()); }
+| <MINUTE> { return new SqlIntervalQualifier(TimeUnit.MINUTE, null,
getPos()); }
+| <HOUR> { return new SqlIntervalQualifier(TimeUnit.HOUR, null, getPos()); }
+| <DAY> { return new SqlIntervalQualifier(TimeUnit.DAY, null, getPos()); }
+| <DOW> { return new SqlIntervalQualifier(TimeUnit.DOW, null, getPos()); }
+| <DOY> { return new SqlIntervalQualifier(TimeUnit.DOY, null, getPos()); }
+| <ISODOW> { return new SqlIntervalQualifier(TimeUnit.ISODOW, null,
getPos()); }
+| <ISOYEAR> { return new SqlIntervalQualifier(TimeUnit.ISOYEAR, null,
getPos()); }
+| <WEEK> { span = span(); }
(
- <NANOSECOND> { return TimeUnit.NANOSECOND; }
- | <MICROSECOND> { return TimeUnit.MICROSECOND; }
+ LOOKAHEAD(2)
+ <LPAREN> w = weekdayName() <RPAREN> {
+ return new SqlIntervalQualifier(w, span.end(this));
+ }
+ |
+ { return new SqlIntervalQualifier(TimeUnit.WEEK, null, getPos()); }
)
-|
- unit = TimeUnit() { return unit; }
+| <MONTH> { return new SqlIntervalQualifier(TimeUnit.MONTH, null, getPos());
}
+| <QUARTER> { return new SqlIntervalQualifier(TimeUnit.QUARTER, null,
getPos()); }
+| <YEAR> { return new SqlIntervalQualifier(TimeUnit.YEAR, null, getPos()); }
+| <EPOCH> { return new SqlIntervalQualifier(TimeUnit.EPOCH, null, getPos());
}
+| <DECADE> { return new SqlIntervalQualifier(TimeUnit.DECADE, null,
getPos()); }
+| <CENTURY> { return new SqlIntervalQualifier(TimeUnit.CENTURY, null,
getPos()); }
+| <MILLENNIUM> { return new SqlIntervalQualifier(TimeUnit.MILLENNIUM, null,
getPos()); }
+| unitName = SimpleIdentifier() {
+ return new SqlIntervalQualifier(unitName.getSimple(),
+ unitName.getParserPosition());
+ }
}
-/**
- * Parses a simple identifier as a TimeUnit.
- */
-TimeUnit TimeUnitIdentifier() :
+String weekdayName() :
{
- final List<String> names = new ArrayList<String>();
- final List<SqlParserPos> positions = new ArrayList<SqlParserPos>();
}
{
- AddIdentifierSegment(names, positions) {
- TimeUnit unit = timeUnitCodes.get(names.get(0));
- if (unit != null) {
- return unit;
- }
- throw SqlUtil.newContextException(positions.get(0),
- RESOURCE.invalidDatetimeFormat(SqlIdentifier.getString(names)));
- }
+ <SUNDAY> { return "WEEK_SUNDAY"; }
Review Comment:
also add little sentence to
https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/functions/systemfunctions/#time-interval-and-point-unit-specifiers
##########
flink-table/flink-table-calcite-bridge/pom.xml:
##########
@@ -45,23 +45,23 @@ under the License.
<version>${calcite.version}</version>
<exclusions>
<!--
- "mvn dependency:tree" as of Calcite 1.32.0:
- [INFO] +-
org.apache.calcite:calcite-core:jar:1.32.0:compile
- [INFO] | +-
org.apache.calcite:calcite-linq4j:jar:1.32.0:compile
+ "mvn dependency:tree" as of Calcite 1.33.0:
+ [INFO] +-
org.apache.calcite:calcite-core:jar:1.33.0:compile
+ [INFO] | +-
org.apache.calcite:calcite-linq4j:jar:1.33.0:compile
[INFO] | +-
org.locationtech.jts:jts-core:jar:1.19.0:compile
- [INFO] | +-
com.fasterxml.jackson.core:jackson-annotations:jar:2.14.3:compile
- [INFO] | +-
org.apache.calcite.avatica:avatica-core:jar:1.22.0:compile
+ [INFO] | +-
com.fasterxml.jackson.core:jackson-annotations:jar:2.15.3:compile
+ [INFO] | +-
org.apache.calcite.avatica:avatica-core:jar:1.23.0:compile
[INFO] | +-
org.apiguardian:apiguardian-api:jar:1.1.2:compile
- [INFO] | +-
com.fasterxml.jackson.core:jackson-core:jar:2.14.3:compile
- [INFO] | +-
com.fasterxml.jackson.core:jackson-databind:jar:2.14.3:compile
+ [INFO] | +-
com.fasterxml.jackson.core:jackson-core:jar:2.15.3:compile
+ [INFO] | +-
com.fasterxml.jackson.core:jackson-databind:jar:2.15.3:compile
[INFO] | +-
com.jayway.jsonpath:json-path:jar:2.7.0:runtime
[INFO] | | \-
net.minidev:json-smart:jar:2.4.7:runtime
[INFO] | | \-
net.minidev:accessors-smart:jar:2.4.7:runtime
[INFO] | | \-
org.ow2.asm:asm:jar:9.1:runtime
[INFO] | +-
commons-codec:commons-codec:jar:1.15:runtime
+ [INFO] | +-
org.apache.commons:commons-math3:jar:3.6.1:runtime
Review Comment:
if we need it, it should be relocated
##########
flink-table/flink-sql-parser/src/main/codegen/templates/Parser.jj:
##########
@@ -6576,11 +6649,127 @@ SqlCall TimestampDiffFunctionCall() :
}
}
+/**
+ * Parses a call to BigQuery's TIMESTAMP_DIFF.
+ *
+ * <p>The difference between TIMESTAMPDIFF and TIMESTAMP_DIFF is the ordering
of
+ * the parameters and the arrangement of the subtraction.
+ * TIMESTAMPDIFF uses (unit, timestamp1, timestamp2) with (t2 - t1), while
+ * TIMESTAMP_DIFF uses (timestamp1, timestamp2, unit) with (t1 - t2).
+ */
+SqlCall TimestampDiff3FunctionCall() :
+{
+ final List<SqlNode> args = new ArrayList<SqlNode>();
+ final Span s;
+ final SqlIntervalQualifier unit;
+}
+{
+ <TIMESTAMP_DIFF> { s = span(); }
+ <LPAREN>
+ AddExpression(args, ExprContext.ACCEPT_SUB_QUERY)
+ <COMMA>
+ AddExpression(args, ExprContext.ACCEPT_SUB_QUERY)
+ <COMMA>
+ unit = TimeUnitOrName() { args.add(unit); }
+ <RPAREN> {
+ return SqlLibraryOperators.TIMESTAMP_DIFF3.createCall(s.end(this),
args);
Review Comment:
check error behavior if we don't support those special parsed functions
##########
flink-table/flink-sql-parser/src/main/codegen/templates/Parser.jj:
##########
@@ -4791,6 +4861,15 @@ TimeUnit Year() :
<YEARS> { return warn(TimeUnit.YEAR); }
}
+TimeUnit Quarter() :
+{
+}
+{
+ <QUARTER> { return TimeUnit.QUARTER; }
+|
+ <QUARTERS> { return warn(TimeUnit.QUARTER); }
Review Comment:
update:
https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/functions/systemfunctions/#time-interval-and-point-unit-specifiers
##########
flink-table/flink-table-planner/src/main/java/org/apache/calcite/rex/RexUtil.java:
##########
@@ -0,0 +1,3176 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.rex;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Range;
+import org.apache.calcite.DataContexts;
+import org.apache.calcite.linq4j.function.Predicate1;
+import org.apache.calcite.plan.RelOptPredicateList;
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.rel.RelCollation;
+import org.apache.calcite.rel.RelCollations;
+import org.apache.calcite.rel.RelFieldCollation;
+import org.apache.calcite.rel.core.Filter;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.Project;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeFamily;
+import org.apache.calcite.rel.type.RelDataTypeField;
+import org.apache.calcite.rex.RexTableInputRef.RelTableRef;
+import org.apache.calcite.sql.SqlAggFunction;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.type.SqlTypeFamily;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeUtil;
+import org.apache.calcite.sql.validate.SqlValidatorUtil;
+import org.apache.calcite.util.ControlFlowException;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.Litmus;
+import org.apache.calcite.util.Pair;
+import org.apache.calcite.util.RangeSets;
+import org.apache.calcite.util.Sarg;
+import org.apache.calcite.util.Util;
+import org.apache.calcite.util.mapping.Mappings;
+import org.apiguardian.api.API;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.function.Predicate;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link org.apache.calcite.rex.RexUtil}, the class
was copied over
+ * because of current Calcite way of inferring constants from IS NOT DISTINCT
FROM clashes with
+ * filter push down.
+ *
+ * <p>Lines 397 ~ 399, Use Calcite 1.32.0 behavior for {@link
RexUtil#gatherConstraints(Class,
Review Comment:
link a Flink ticket to this
##########
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/planner/plan/rules/logical/JoinDependentConditionDerivationRuleTest.xml:
##########
@@ -120,7 +120,7 @@ LogicalProject(a=[$0], d=[$3])
<Resource name="optimized rel plan">
<![CDATA[
LogicalProject(a=[$0], d=[$3])
-+- LogicalJoin(condition=[AND(OR(AND(=($0, 1), =($3, 2)), AND(=($0, 2), =($3,
1))), OR(AND(=($0, 3), =($3, 4)), AND(=($0, 4), =($3, 3))), SEARCH($0, Sarg[1,
2]), SEARCH($3, Sarg[1, 2]), SEARCH($0, Sarg[3, 4]), SEARCH($3, Sarg[3, 4]))],
joinType=[inner])
++- LogicalJoin(condition=[false], joinType=[inner])
Review Comment:
Test:
```
SELECT a, d FROM MyTable1, MyTable2 WHERE ((a = 1 AND d = 2) OR (a = 2 AND d
= 1))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
