Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]
mihaibudiu commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1471799166
##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +258,60 @@ public Result visit(Join e) {
return result(join, leftResult, rightResult);
}
+ private Result maybeFixRenamedFields(Result rightResult, Join e) {
+// TableModify is treated in a special way inside visit(TableModify
modify).
+// Therefore, we need to skip it here
+Frame last = stack.peekLast();
+if (last != null && last.r instanceof TableModify) {
+ return rightResult;
+}
+// If there is a filter on the stack an additional "SELECT * ... AS tx" is
added
+// as wrapper. This could hide the original aliases used inside.
+// So we need to reflect the renaming of the fields in the join condition,
+// which was already done in SqlValidatorUtil.deriveJoinRowType
+if (stack.stream().noneMatch(it -> it.r instanceof Filter)) {
+ return rightResult;
+}
+List rightFieldNames = e.getRight().getRowType().getFieldNames();
+List fieldNames = e.getRowType().getFieldNames();
+int offset = e.getLeft().getRowType().getFieldCount();
+boolean hasFieldNameCollision = false;
+for (int i = 0; i < rightFieldNames.size(); i++) {
+ if (!rightFieldNames.get(i).equals(fieldNames.get(offset + i))) {
Review Comment:
why is it sufficient to compare with the field with the same relative index
rather than compare all n^2 pairs?
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
(calcite) branch main updated: [CALCITE-6208] Update JSON_VALUE return type inference to make explicit array return types be nullable with nullable elements
This is an automated email from the ASF dual-hosted git repository.
mbudiu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/main by this push:
new de39888754 [CALCITE-6208] Update JSON_VALUE return type inference to
make explicit array return types be nullable with nullable elements
de39888754 is described below
commit de39888754759d3e6b7ce35770fcff3d8d0e6a4c
Author: Clint Wylie
AuthorDate: Wed Jan 17 00:30:19 2024 -0800
[CALCITE-6208] Update JSON_VALUE return type inference to make explicit
array return types be nullable with nullable elements
---
.../calcite/sql/fun/SqlJsonValueFunction.java | 24 --
.../org/apache/calcite/test/SqlValidatorTest.java | 8
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git
a/core/src/main/java/org/apache/calcite/sql/fun/SqlJsonValueFunction.java
b/core/src/main/java/org/apache/calcite/sql/fun/SqlJsonValueFunction.java
index 5cc542bb49..84bc1bad68 100644
--- a/core/src/main/java/org/apache/calcite/sql/fun/SqlJsonValueFunction.java
+++ b/core/src/main/java/org/apache/calcite/sql/fun/SqlJsonValueFunction.java
@@ -30,11 +30,10 @@ import org.apache.calcite.sql.SqlOperandCountRange;
import org.apache.calcite.sql.SqlOperatorBinding;
import org.apache.calcite.sql.SqlWriter;
import org.apache.calcite.sql.type.OperandTypes;
-import org.apache.calcite.sql.type.ReturnTypes;
import org.apache.calcite.sql.type.SqlOperandCountRanges;
import org.apache.calcite.sql.type.SqlTypeFamily;
import org.apache.calcite.sql.type.SqlTypeName;
-import org.apache.calcite.sql.type.SqlTypeTransforms;
+import org.apache.calcite.sql.type.SqlTypeUtil;
import com.google.common.collect.ImmutableList;
@@ -54,9 +53,9 @@ public class SqlJsonValueFunction extends SqlFunction {
public SqlJsonValueFunction(String name) {
super(name, SqlKind.OTHER_FUNCTION,
-ReturnTypes.cascade(
-opBinding ->
explicitTypeSpec(opBinding).orElse(getDefaultType(opBinding)),
-SqlTypeTransforms.FORCE_NULLABLE),
+opBinding -> explicitTypeSpec(opBinding)
+.map(relDataType -> deriveExplicitType(opBinding, relDataType))
+.orElseGet(() -> getDefaultType(opBinding)),
null,
OperandTypes.family(
ImmutableList.of(SqlTypeFamily.ANY, SqlTypeFamily.CHARACTER),
@@ -64,10 +63,23 @@ public class SqlJsonValueFunction extends SqlFunction {
SqlFunctionCategory.SYSTEM);
}
+ private static RelDataType deriveExplicitType(SqlOperatorBinding opBinding,
RelDataType type) {
+if (SqlTypeName.ARRAY == type.getSqlTypeName()) {
+ RelDataType elementType =
Objects.requireNonNull(type.getComponentType());
+ RelDataType nullableElementType = deriveExplicitType(opBinding,
elementType);
+ return SqlTypeUtil.createArrayType(
+ opBinding.getTypeFactory(),
+ nullableElementType,
+ true);
+}
+return opBinding.getTypeFactory().createTypeWithNullability(type, true);
+ }
+
/** Returns VARCHAR(2000) as default. */
private static RelDataType getDefaultType(SqlOperatorBinding opBinding) {
final RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
-return typeFactory.createSqlType(SqlTypeName.VARCHAR, 2000);
+final RelDataType baseType =
typeFactory.createSqlType(SqlTypeName.VARCHAR, 2000);
+return typeFactory.createTypeWithNullability(baseType, true);
}
/**
diff --git a/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
b/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
index fff0fc26e2..8fead2ce72 100644
--- a/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
+++ b/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
@@ -11417,6 +11417,14 @@ public class SqlValidatorTest extends
SqlValidatorTestCase {
expr("json_value('{\"foo\":100}', 'lax $.foo' returning boolean"
+ " default 100 on empty)")
.columnType("BOOLEAN");
+
+expr("json_value('{\"foo\":[100, null, 200]}', 'lax $.foo'"
++ "returning integer array)")
+.columnType("INTEGER ARRAY");
+
+expr("json_value('{\"foo\":[[100, null, 200]]}', 'lax $.foo'"
++ "returning integer array array)")
+.columnType("INTEGER ARRAY ARRAY");
}
@Test void testJsonQuery() {
Re: [PR] [CALCITE-6208] update JSON_VALUE return type inference to make explicit array return types be nullable with nullable elements [calcite]
mihaibudiu merged PR #3633: URL: https://github.com/apache/calcite/pull/3633 -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6224] Add LOG2 function (enabled in Mysql, Spark library) [calcite]
tanclary commented on code in PR #3648:
URL: https://github.com/apache/calcite/pull/3648#discussion_r1471659722
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -6188,6 +6188,65 @@ void checkRegexpExtract(SqlOperatorFixture f0,
FunctionAlias functionAlias) {
f.checkNull("log(10, cast(null as real))");
}
+ /** Test case for
+ * https://issues.apache.org/jira/browse/CALCITE-6224;>[CALCITE-6224]
+ * Add LOG@ function (enabled in MYSQL, Spark library). */
+ @Test void testLog2Func() {
+final SqlOperatorFixture f0 = fixture();
+final Consumer consumer = f -> {
+ f.setFor(SqlLibraryOperators.LOG2);
+ f.checkScalarApprox("log2(2)", "DOUBLE NOT NULL",
+ isWithin(1.0, 0.01));
+ f.checkScalarApprox("log2(4)", "DOUBLE NOT NULL",
+ isWithin(2.0, 0.01));
+ f.checkScalarApprox("log2(65536)", "DOUBLE NOT NULL",
+ isWithin(16.0, 0.01));
+ f.checkScalarApprox("log2(-2)", "DOUBLE NOT NULL",
+ "NaN");
+ f.checkScalarApprox("log2(2/3)", "DOUBLE NOT NULL",
+ "-Infinity");
+ f.checkScalarApprox("log2(2.2)", "DOUBLE NOT NULL",
+ "1.1375035237499351");
+ f.checkScalarApprox("log2(0.5)", "DOUBLE NOT NULL",
+ "-1.0");
+ f.checkScalarApprox("log2(3)", "DOUBLE NOT NULL",
+ isWithin(1.5849625007211563, 0.01));
+ f.checkNull("log2(cast(null as real))");
+};
+f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.SPARK), consumer);
+ }
+
+ /** Test case for
+ * https://issues.apache.org/jira/browse/CALCITE-6232;>[CALCITE-6232]
+ * Using fractions in LOG function does not return correct results. */
+ @Test void testLogFuncByConvert() {
+// The fractional conversion of the Log function is reserved only for
integer bits
+final SqlOperatorFixture f0 = Fixtures.forOperators(true);
+f0.setFor(SqlLibraryOperators.LOG, VmName.EXPAND);
+final SqlOperatorFixture f = f0.withLibrary(SqlLibrary.BIG_QUERY);
+f.checkScalarApprox("log(2/3, 2)", "DOUBLE NOT NULL",
+ "-Infinity");
+f.checkScalarApprox("log(0,2)", "DOUBLE NOT NULL",
+ "-Infinity");
+f.checkScalarApprox("log(1,2)", "DOUBLE NOT NULL",
+"0.0");
+f.checkScalarApprox("log(4/3,2)", "DOUBLE NOT NULL",
Review Comment:
What happens when you wrap the arguments in casts?
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -6188,6 +6188,65 @@ void checkRegexpExtract(SqlOperatorFixture f0,
FunctionAlias functionAlias) {
f.checkNull("log(10, cast(null as real))");
}
+ /** Test case for
+ * https://issues.apache.org/jira/browse/CALCITE-6224;>[CALCITE-6224]
+ * Add LOG@ function (enabled in MYSQL, Spark library). */
+ @Test void testLog2Func() {
+final SqlOperatorFixture f0 = fixture();
+final Consumer consumer = f -> {
+ f.setFor(SqlLibraryOperators.LOG2);
+ f.checkScalarApprox("log2(2)", "DOUBLE NOT NULL",
+ isWithin(1.0, 0.01));
+ f.checkScalarApprox("log2(4)", "DOUBLE NOT NULL",
+ isWithin(2.0, 0.01));
+ f.checkScalarApprox("log2(65536)", "DOUBLE NOT NULL",
+ isWithin(16.0, 0.01));
+ f.checkScalarApprox("log2(-2)", "DOUBLE NOT NULL",
+ "NaN");
+ f.checkScalarApprox("log2(2/3)", "DOUBLE NOT NULL",
+ "-Infinity");
+ f.checkScalarApprox("log2(2.2)", "DOUBLE NOT NULL",
+ "1.1375035237499351");
+ f.checkScalarApprox("log2(0.5)", "DOUBLE NOT NULL",
+ "-1.0");
+ f.checkScalarApprox("log2(3)", "DOUBLE NOT NULL",
+ isWithin(1.5849625007211563, 0.01));
+ f.checkNull("log2(cast(null as real))");
+};
+f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.SPARK), consumer);
+ }
+
+ /** Test case for
+ * https://issues.apache.org/jira/browse/CALCITE-6232;>[CALCITE-6232]
+ * Using fractions in LOG function does not return correct results. */
+ @Test void testLogFuncByConvert() {
+// The fractional conversion of the Log function is reserved only for
integer bits
+final SqlOperatorFixture f0 = Fixtures.forOperators(true);
+f0.setFor(SqlLibraryOperators.LOG, VmName.EXPAND);
+final SqlOperatorFixture f = f0.withLibrary(SqlLibrary.BIG_QUERY);
+f.checkScalarApprox("log(2/3, 2)", "DOUBLE NOT NULL",
+ "-Infinity");
+f.checkScalarApprox("log(0,2)", "DOUBLE NOT NULL",
+ "-Infinity");
+f.checkScalarApprox("log(1,2)", "DOUBLE NOT NULL",
+"0.0");
+f.checkScalarApprox("log(4/3,2)", "DOUBLE NOT NULL",
+"0.0");
+f.checkScalarApprox("log(5/3,2)", "DOUBLE NOT NULL",
+"0.0");
+f.checkScalarApprox("log(2/3, 10)", "DOUBLE NOT NULL",
+ "-Infinity");
+f.checkScalarApprox("log(0,10)", "DOUBLE NOT NULL",
+ "-Infinity");
+f.checkScalarApprox("log(1,10)", "DOUBLE NOT NULL",
+"0.0");
+f.checkScalarApprox("log(4/3,10)",
Re: [PR] [CALCITE-6223] Add MAP_CONTAINS_KEY function (enabled in Spark library) [calcite]
tanclary commented on code in PR #3655:
URL: https://github.com/apache/calcite/pull/3655#discussion_r1471624089
##
core/src/main/java/org/apache/calcite/sql/type/MapKeyOperandTypeChecker.java:
##
@@ -0,0 +1,73 @@
+/*
+ * 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.sql.type;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlOperandCountRange;
+import org.apache.calcite.sql.SqlOperator;
+
+import com.google.common.collect.ImmutableList;
+
+import static
org.apache.calcite.sql.type.NonNullableAccessors.getKeyTypeOrThrow;
+import static org.apache.calcite.util.Static.RESOURCE;
+
+/**
+ * Parameter type-checking strategy where types must be Map and Map key type.
+ */
+public class MapKeyOperandTypeChecker implements SqlOperandTypeChecker {
Review Comment:
Do we need an entire new class for this? We should just add a new entry to
`OperandTypes` if possible. Or at least use a nested class (like
`PeriodOperandTypeChecker`)
##
site/_docs/reference.md:
##
@@ -2794,6 +2794,7 @@ In the following:
| s | MAP() | Returns an empty map
| s | MAP(key, value [, key, value]*)| Returns a map with the
given *key*/*value* pairs
| s | MAP_CONCAT(map [, map]*) | Concatenates one or
more maps. If any input argument is `NULL` the function returns `NULL`. Note
that calcite is using the LAST_WIN strategy
+| s | MAP_CONTAINS_KEY(map, key) | Returns true if the
*map* contains the *key*
Review Comment:
What does it return if the map does not contain the key? False? Null? Error?
Maybe `Returns whether map contains key* is better because it has less
ambiguity.
##
core/src/main/java/org/apache/calcite/sql/type/MapKeyOperandTypeChecker.java:
##
@@ -0,0 +1,73 @@
+/*
+ * 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.sql.type;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlOperandCountRange;
+import org.apache.calcite.sql.SqlOperator;
+
+import com.google.common.collect.ImmutableList;
+
+import static
org.apache.calcite.sql.type.NonNullableAccessors.getKeyTypeOrThrow;
+import static org.apache.calcite.util.Static.RESOURCE;
+
+/**
+ * Parameter type-checking strategy where types must be Map and Map key type.
+ */
+public class MapKeyOperandTypeChecker implements SqlOperandTypeChecker {
+ @Override public boolean checkOperandTypes(
+ SqlCallBinding callBinding,
+ boolean throwOnFailure) {
+final SqlNode op0 = callBinding.operand(0);
+if (!OperandTypes.MAP.checkSingleOperandType(
+callBinding,
+op0,
Review Comment:
I don't really think we should have to write all of this out. What is
specific to MAP and KEY that it needs unique handling? From what I see you're
just verifying the type of the 1st and 2nd arguments. Let me know if I'm
missing something.
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -7189,6 +7189,43 @@ void checkRegexpExtract(SqlOperatorFixture f0,
FunctionAlias functionAlias) {
"INTEGER ARRAY NOT NULL");
}
+ /** Test case for
+ *
Re: [PR] [CALCITE-6234] Add Test on SqlOperatorTest for format_date function(enable Bigquery and pg) [calcite]
sonarcloud[bot] commented on PR #3659: URL: https://github.com/apache/calcite/pull/3659#issuecomment-1917357695 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3659) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [6 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3659=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3659=false=true) [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3659=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3659=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3659) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6234] Add Test on SqlOperatorTest for format_date function(enable Bigquery and pg) [calcite]
sonarcloud[bot] commented on PR #3659: URL: https://github.com/apache/calcite/pull/3659#issuecomment-1917326570 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3659) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [6 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3659=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3659=false=true) [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3659=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3659=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3659) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6223] Add MAP_CONTAINS_KEY function (enabled in Spark library) [calcite]
caicancai commented on code in PR #3655:
URL: https://github.com/apache/calcite/pull/3655#discussion_r1471485862
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -7189,6 +7189,33 @@ void checkRegexpExtract(SqlOperatorFixture f0,
FunctionAlias functionAlias) {
"INTEGER ARRAY NOT NULL");
}
+ @Test void testMapContainsKeyFunc() {
Review Comment:
https://issues.apache.org/jira/browse/CALCITE-6235
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -7189,6 +7189,33 @@ void checkRegexpExtract(SqlOperatorFixture f0,
FunctionAlias functionAlias) {
"INTEGER ARRAY NOT NULL");
}
+ @Test void testMapContainsKeyFunc() {
Review Comment:
@macroguo-ghy @mihaibudiu https://issues.apache.org/jira/browse/CALCITE-6235
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6234] Add Test on SqlOperatorTest for format_date function(e… [calcite]
caicancai commented on PR #3659: URL: https://github.com/apache/calcite/pull/3659#issuecomment-1917210942 I have to admit that this is a problem left by my previous PR, and I'm sorry 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. To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [CALCITE-6234] Add Test on SqlOperatorTest for format_date function(e… [calcite]
caicancai opened a new pull request, #3659: URL: https://github.com/apache/calcite/pull/3659 …nable Bigquery and pg) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]
sonarcloud[bot] commented on PR #3658: URL: https://github.com/apache/calcite/pull/3658#issuecomment-1916821078 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3658) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite=3658=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3658=false=true) [98.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3658) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]
sonarcloud[bot] commented on PR #3658: URL: https://github.com/apache/calcite/pull/3658#issuecomment-1916812639 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3658) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite=3658=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3658=false=true) [98.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3658) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6226] Wrong ISOWEEK and no ISOYEAR on BigQuery FORMAT_DATE [calcite]
rorueda commented on code in PR #3653:
URL: https://github.com/apache/calcite/pull/3653#discussion_r1471099647
##
core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java:
##
@@ -132,17 +133,41 @@ public enum FormatElementEnum implements FormatElement {
sb.append(String.format(Locale.ROOT, "%02d",
calendar.get(Calendar.HOUR_OF_DAY)));
}
},
+ ID("u", "The weekday (Monday as the first day of the week) as a decimal
number (1-7)") {
+@Override public void format(StringBuilder sb, Date date) {
+ final Calendar calendar = Work.get().calendar;
+ calendar.setTime(date);
+ int weekDay = calendar.get(Calendar.DAY_OF_WEEK);
+ sb.append(String.format(Locale.ROOT, "%d", weekDay == 1 ? 7 : weekDay -
1));
Review Comment:
Added a comment - don't think my suggestion would make it clearer.
Looking at Postgres documentation, the `EXTRACT(DOW...` assumes Sunday = 0,
but the format functions `to_char, to_date,...` also assume Sunday = 1 for day
of week.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6226] Wrong ISOWEEK and no ISOYEAR on BigQuery FORMAT_DATE [calcite]
sonarcloud[bot] commented on PR #3653: URL: https://github.com/apache/calcite/pull/3653#issuecomment-1916697183 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3653) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [6 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3653=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3653=false=true) [83.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3653=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3653=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3653) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6226] Wrong ISOWEEK and no ISOYEAR on BigQuery FORMAT_DATE [calcite]
rorueda commented on code in PR #3653:
URL: https://github.com/apache/calcite/pull/3653#discussion_r1471071836
##
core/src/test/java/org/apache/calcite/util/format/FormatElementEnumTest.java:
##
@@ -77,8 +80,26 @@ class FormatElementEnumTest {
assertFormatElement(FormatElementEnum.FF6, "2014-09-30T10:00:00.123456Z",
"000123");
}
- @Test void testIW() {
-assertFormatElement(FormatElementEnum.IW, "2014-09-30T10:00:00Z", "40");
+ @Test void testID() {
+assertFormatElement(FormatElementEnum.ID, "2014-09-30T10:00:00Z", "2");
+ }
Review Comment:
As there is one method for each format element, I didn't want to add new
methods to test the edge cases of ISOWEEK and ISOYEAR, and I thought a good
solution was to make those tests parameterized.
But I agree that the way it was done didn't help in making the added test
cases understandable. I removed the parameters and made more assertions inside
the method (with a comment).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6231] JDBC adapter generates "UNNEST" when it should generate "UNNEST ... WITH ORDINALITY" [calcite]
sonarcloud[bot] commented on PR #3654: URL: https://github.com/apache/calcite/pull/3654#issuecomment-1916655269 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3654) **Quality Gate passed** Kudos, no new issues were introduced! [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3654=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3654=false=true) [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3654=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3654=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3654) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6226] Wrong ISOWEEK and no ISOYEAR on BigQuery FORMAT_DATE [calcite]
rorueda commented on code in PR #3653:
URL: https://github.com/apache/calcite/pull/3653#discussion_r1471054084
##
core/src/main/java/org/apache/calcite/util/format/FormatElementEnum.java:
##
@@ -48,7 +48,8 @@ public enum FormatElementEnum implements FormatElement {
sb.append(String.format(Locale.ROOT, "%2d", calendar.get(Calendar.YEAR)
/ 100 + 1));
}
},
- D("F", "The weekday (Monday as the first day of the week) as a decimal
number (1-7)") {
+ // TODO: Parsing of weekdays with sunday as first day of the week
Review Comment:
Created CALCITE-6233 to handle the parsing of ISOWEEK.
It doesn't seem that parsing the weekday with Sunday as first day (1) is
possible with java's DateFormat.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6224] Add LOG2 function (enabled in Mysql, Spark library) [calcite]
sonarcloud[bot] commented on PR #3648: URL: https://github.com/apache/calcite/pull/3648#issuecomment-1916583880 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3648) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [14 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3648=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3648=false=true) [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3648=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3648=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3648) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6223] Add MAP_CONTAINS_KEY function (enabled in Spark library) [calcite]
sonarcloud[bot] commented on PR #3655: URL: https://github.com/apache/calcite/pull/3655#issuecomment-1916542382 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3655) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [15 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3655=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3655=false=true) [88.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3655=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3655=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3655) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6224] Add LOG2 function (enabled in Mysql, Spark library) [calcite]
caicancai commented on code in PR #3648:
URL: https://github.com/apache/calcite/pull/3648#discussion_r1470970503
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -6188,6 +6188,29 @@ void checkRegexpExtract(SqlOperatorFixture f0,
FunctionAlias functionAlias) {
f.checkNull("log(10, cast(null as real))");
}
+ @Test void testLog2Func() {
+final SqlOperatorFixture f0 = Fixtures.forOperators(true);
+f0.setFor(SqlLibraryOperators.LOG2, VmName.EXPAND);
+f0.checkFails("^log2(4)^",
+"No match found for function signature LOG2\\(\\)", false);
+final SqlOperatorFixture f = f0.withLibrary(SqlLibrary.MYSQL);
+f.checkScalarApprox("log2(2)", "DOUBLE NOT NULL",
+isWithin(1.0, 0.01));
+f.checkScalarApprox("log2(4)", "DOUBLE NOT NULL",
+isWithin(2.0, 0.01));
+f.checkScalarApprox("log2(65536)", "DOUBLE NOT NULL",
+isWithin(16.0, 0.01));
+f.checkScalarApprox("log2(-2)", "DOUBLE NOT NULL",
+"NaN");
+f.checkScalarApprox("log2(2/3)", "DOUBLE NOT NULL",
Review Comment:
@tanclary I specially added a test for this bug, I believe that users and
developers can understand
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6223] Add MAP_CONTAINS_KEY function (enabled in Spark library) [calcite]
caicancai commented on code in PR #3655: URL: https://github.com/apache/calcite/pull/3655#discussion_r1470834337 ## core/src/main/java/org/apache/calcite/sql/type/MapKeyOperandTypeChecker.java: ## @@ -0,0 +1,73 @@ +/* + * 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.sql.type; + +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.sql.SqlCallBinding; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.SqlOperandCountRange; +import org.apache.calcite.sql.SqlOperator; + +import com.google.common.collect.ImmutableList; + +import static org.apache.calcite.sql.type.NonNullableAccessors.getKeyTypeOrThrow; +import static org.apache.calcite.util.Static.RESOURCE; + +/** + * Parameter type-checking strategy where types must be Map and Map key type. Review Comment: I tried to change it to Parameter type-checking strategy where types must be Map and K type. But build reported an error -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6223] Add MAP_CONTAINS_KEY function (enabled in Spark library) [calcite]
caicancai commented on code in PR #3655:
URL: https://github.com/apache/calcite/pull/3655#discussion_r1470830103
##
core/src/main/java/org/apache/calcite/sql/type/MapKeyOperandTypeChecker.java:
##
@@ -0,0 +1,73 @@
+/*
+ * 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.sql.type;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlOperandCountRange;
+import org.apache.calcite.sql.SqlOperator;
+
+import com.google.common.collect.ImmutableList;
+
+import static
org.apache.calcite.sql.type.NonNullableAccessors.getKeyTypeOrThrow;
+import static org.apache.calcite.util.Static.RESOURCE;
+
+/**
+ * Parameter type-checking strategy where types must be Map and Map key type.
+ */
+public class MapKeyOperandTypeChecker implements SqlOperandTypeChecker {
+ @Override public boolean checkOperandTypes(
+ SqlCallBinding callBinding,
+ boolean throwOnFailure) {
+final SqlNode op0 = callBinding.operand(0);
+if (!OperandTypes.MAP.checkSingleOperandType(
+callBinding,
+op0,
+0,
+throwOnFailure)) {
+ return false;
+}
+
+final RelDataType mapComponentType =
+getKeyTypeOrThrow(SqlTypeUtil.deriveType(callBinding, op0));
+final SqlNode op1 = callBinding.operand(1);
+RelDataType mapType1 = SqlTypeUtil.deriveType(callBinding, op1);
+
+RelDataType biggest =
+callBinding.getTypeFactory().leastRestrictive(
+ImmutableList.of(mapComponentType, mapType1));
+if (biggest == null) {
+ if (throwOnFailure) {
+throw callBinding.newError(
+RESOURCE.typeNotComparable(
+mapComponentType.toString(), mapType1.toString()));
+ }
+
+ return false;
+}
+return true;
+ }
+
+ @Override public SqlOperandCountRange getOperandCountRange() {
+return SqlOperandCountRanges.of(2);
+ }
+
+ @Override public String getAllowedSignatures(SqlOperator op, String opName) {
Review Comment:
I am not sure what the specific function is here, can you tell me how to
modify 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.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6223] Add MAP_CONTAINS_KEY function (enabled in Spark library) [calcite]
caicancai commented on code in PR #3655:
URL: https://github.com/apache/calcite/pull/3655#discussion_r1470828901
##
core/src/main/java/org/apache/calcite/sql/type/NonNullableAccessors.java:
##
@@ -52,4 +52,10 @@ public static RelDataType
getComponentTypeOrThrow(RelDataType type) {
return requireNonNull(type.getComponentType(),
() -> "componentType is null for " + type);
}
+
+ @API(since = "1.37", status = API.Status.EXPERIMENTAL)
Review Comment:
Honestly, I don't know. I just borrowed it from the above
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]
kramerul commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1470726277
##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +257,48 @@ public Result visit(Join e) {
return result(join, leftResult, rightResult);
}
+ private Result maybeFixRenamedFields(Result rightResult, Join e) {
+Frame last = stack.peekLast();
+if (last != null && last.r instanceof TableModify) {
+ return rightResult;
+}
+List rightFieldNames = e.getRight().getRowType().getFieldNames();
+List fieldNames = e.getRowType().getFieldNames();
+int offset = e.getLeft().getRowType().getFieldCount();
+boolean hasFieldNameCollision = false;
+for (int i = 0; i < rightFieldNames.size(); i++) {
+ if (!rightFieldNames.get(i).equals(fieldNames.get(offset + i))) {
+hasFieldNameCollision = true;
+ }
+}
+if (!hasFieldNameCollision) {
+ return rightResult;
+}
+Builder builder = rightResult.builder(e);
+List oldSelectList = new ArrayList<>();
+if (builder.select.getSelectList() == SqlNodeList.SINGLETON_STAR) {
+ for (int i = 0; i < rightFieldNames.size(); i++) {
+oldSelectList.add(new SqlIdentifier(rightFieldNames.get(i), POS));
+ }
+} else {
+ for (SqlNode node : builder.select.getSelectList().getList()) {
+oldSelectList.add(requireNonNull(node, "node"));
+ }
+}
+List selectList = new ArrayList<>();
+for (int i = 0; i < rightFieldNames.size(); i++) {
+ SqlNode column = oldSelectList.get(i);
+ if (!rightFieldNames.get(i).equals(fieldNames.get(offset + i))) {
+column =
+SqlStdOperatorTable.AS.createCall(POS, SqlUtil.stripAs(column),
Review Comment:
The uniqueness is currently guaranteed by the function
`SqlValidatorUtil.deriveJoinRowType` which creates the unique field names for
the `Join` relation. But this is not really visible here. Therefore, I will add
a comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
