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 check.
--
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_r1470720546
##
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##
@@ -809,8 +809,8 @@ private static String toSql(RelNode root, SqlDialect
dialect,
// RelFieldTrimmer maybe build the RelNode.
relFn(fn).ok("SELECT \"t\".\"V\" AS \"l_v\"\n"
+ "FROM (VALUES (1, 2)) AS \"t\" (\"K\", \"V\")\n"
-+ "INNER JOIN "
-+ "(VALUES (1)) AS \"t0\" (\"K\") ON \"t\".\"K\" = \"t0\".\"K\"");
++ "INNER JOIN (SELECT \"K\" AS \"K0\"\n"
Review Comment:
Yes it's quite hard to review all changes. I also had a look at most of the
changes inside the 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.
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_r1470719490
##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -225,7 +225,7 @@ public Result visit(Join e) {
break;
}
final Result leftResult = visitInput(e, 0).resetAlias();
-final Result rightResult = visitInput(e, 1).resetAlias();
+Result rightResult = maybeFixRenamedFields(visitInput(e, 1).resetAlias(),
e);
Review Comment:
I changed this.
--
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_r1470712364
##
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) {
Review Comment:
The class `RelToSqlConverter` is only used in `JdbcImplementor` and
`PigRelToSqlConverter`. Both classes are related to JDBC. Therefore, I would
like to keep the title of the issue. Additionally Julian Hyde added the JDBC
relation to the title
--
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-6208] update JSON_VALUE return type inference to make explicit array return types be nullable with nullable elements [calcite]
clintropolis commented on PR #3633: URL: https://github.com/apache/calcite/pull/3633#issuecomment-1916063382 updated the commit message, not sure why CI is still failing -- 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-1916040904 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3655) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [16 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) [76.0% 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-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
mihaibudiu commented on PR #3632: URL: https://github.com/apache/calcite/pull/3632#issuecomment-1915788785 Supposedly you have changed this behavior because it causes some problems in some application someone cares about. The ideal test case would be a small reproduction of the bug which caused this quest in the first place. The JIRA cases suggests that a small reproduction is difficult. -- 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-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
jduo commented on PR #3632: URL: https://github.com/apache/calcite/pull/3632#issuecomment-1915785837 > You are describing how something is implemented. A bug description should be something like "Query X produces result Y instead of the expected Z", or "Optimizer produces the plan X instead of the expected Y because it uses the wrong statistics". I guess in this case, the original implementation happens to return the correct value, but is not behaving as a valid extension point. So I'm not sure what would be a good concrete test for this type of problem. -- 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-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
mihaibudiu commented on PR #3632: URL: https://github.com/apache/calcite/pull/3632#issuecomment-1915767458 You are describing how something is implemented. A bug description should be something like "Query X produces result Y instead of the expected Z", or "Optimizer produces the plan X instead of the expected Y because it uses the wrong statistics". -- 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-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
jduo commented on PR #3632: URL: https://github.com/apache/calcite/pull/3632#issuecomment-1915761224 > I am approving, but I am still unsure about exactly what this improvement fixes. The intent with RelMetadataQuery is that the implementation can provide custom behavior for each of the statistics you can requet with it. For the case of the population size of Values clauses, it should be based on the row count / 2. However RelMetadataQuery was not using invoking its (potentially custom) method to get the row count, it was just inlining the row count estimate from the node. -- 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-6228] ELEMENT function infers incorrect return type [calcite]
sonarcloud[bot] commented on PR #3650: URL: https://github.com/apache/calcite/pull/3650#issuecomment-1915509743 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3650) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [13 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3650=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3650=false=true) [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3650=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3650=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3650) -- 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]
julianhyde commented on code in PR #3653:
URL: https://github.com/apache/calcite/pull/3653#discussion_r1470137795
##
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:
Please remove the TODO, log a jira case.
##
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:
I don't understand why some of the tests in this class are parameterized and
others are not.
Frankly I don't think the framework is helping make the tests more
understandable. I would move to code, and add comments if a test is testing
something subtle.
It seems to me that this test class should be very simple.
--
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]
mihaibudiu commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1470106502
##
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:
What guarantees that this name is unique?
This may be another column name that appears on the other side.
I think you need to create a Set with all "forbidden" names and make sure
that this name is not part of that set.
##
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) {
Review Comment:
This doesn't seem to have any relation to JDBC.
Perhaps the issue should be renamed.
##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -225,7 +225,7 @@ public Result visit(Join e) {
break;
}
final Result leftResult = visitInput(e, 0).resetAlias();
-final Result rightResult = visitInput(e, 1).resetAlias();
+Result rightResult = maybeFixRenamedFields(visitInput(e, 1).resetAlias(),
e);
Review Comment:
Can these two calls be on different lines? Helps with debugging.
Looks like you can keep the "final" too.
##
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) {
Review Comment:
I suspect that this change may affect other open issues on JIRA, it looks
pretty basic.
Can you do a quick check?
##
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##
@@ -809,8 +809,8 @@ private static String toSql(RelNode root, SqlDialect
dialect,
// RelFieldTrimmer maybe build the RelNode.
relFn(fn).ok("SELECT \"t\".\"V\" AS \"l_v\"\n"
+ "FROM (VALUES (1, 2)) AS \"t\" (\"K\", \"V\")\n"
-+ "INNER JOIN "
-+ "(VALUES (1)) AS \"t0\" (\"K\") ON \"t\".\"K\" = \"t0\".\"K\"");
++ "INNER JOIN (SELECT \"K\" AS \"K0\"\n"
Review Comment:
It's not easy to review all these changes. I checked some of them.
--
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-6228] ELEMENT function infers incorrect return type [calcite]
mihaibudiu commented on code in PR #3650:
URL: https://github.com/apache/calcite/pull/3650#discussion_r1470101788
##
core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java:
##
@@ -615,10 +615,11 @@ public static SqlCall stripSeparator(SqlCall call) {
ARG0.andThen(SqlTypeTransforms.TO_MULTISET);
/**
- * Returns the element type of a MULTISET.
+ * Returns the element type of a MULTISET, with nullability enforced.
*/
public static final SqlReturnTypeInference MULTISET_ELEMENT_NULLABLE =
Review Comment:
@JiajunBernoulli done rebasing/squashing. Thank 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.
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-6210] Cast to VARBINARY causes an assertion failure [calcite]
mihaibudiu commented on code in PR #3635:
URL: https://github.com/apache/calcite/pull/3635#discussion_r1468522157
##
core/src/main/java/org/apache/calcite/rex/RexUtil.java:
##
@@ -1683,7 +1683,7 @@ public static boolean isLosslessCast(RexNode node) {
*
* @param source source type
* @param target target type
- * @return true iff the conversion is a loss-less cast
+ * @return true if the conversion is a loss-less cast
Review Comment:
I know, but this function does not return "true" if and only if, it returns
it only "if", because it is an aproximation.
So I think the original comment was wrong.
--
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-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
mihaibudiu commented on code in PR #3632:
URL: https://github.com/apache/calcite/pull/3632#discussion_r1470092970
##
core/src/test/java/org/apache/calcite/test/RelMetadataTest.java:
##
@@ -3639,6 +3642,37 @@ public void checkAllPredicatesAndTableSetOp(String sql) {
is(mq.getPopulationSize(rel, bitSetOf(0;
}
+ /**
+ * Test that RelMdPopulationSize is calculated based on the
RelMetadataQuery#getRowCount().
+ *
+ * @see https://issues.apache.org/jira/browse/CALCITE-5647;>[CALCITE-5647]
+ */
+ @Test public void testPopulationSizeFromValues() {
Review Comment:
Yes, the test is still artificial.
--
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]
mihaibudiu commented on code in PR #3655:
URL: https://github.com/apache/calcite/pull/3655#discussion_r1470025061
##
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:
Does anyone know what the API annotation is for?
Is the EXPERIMENTAL tag ever removed?
##
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##
@@ -844,6 +845,7 @@ Builder populate2() {
defineMethod(ARRAYS_ZIP, BuiltInMethod.ARRAYS_ZIP.method,
NullPolicy.ANY);
defineMethod(EXISTS, BuiltInMethod.EXISTS.method, NullPolicy.ANY);
defineMethod(MAP_CONCAT, BuiltInMethod.MAP_CONCAT.method,
NullPolicy.ANY);
+ defineMethod(MAP_CONTAINS_KEYS, BuiltInMethod.MAP_CONTAINS_KEY.method,
NullPolicy.ANY);
Review Comment:
why KEYS and KEY? Shouldn't they match?
##
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 this is used for, but this string also looks wrong.
##
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
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_r1469962583
##
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:
I mean it's definitely a bug but you may want to rewrite your case such that
the issue is more clearly stated. I would say stick with a single test case,
give the expected results and the actual results. Feel free to tag me on any
cases or PRs you file.
I should have found this during my previous LOG work but I didn't. So let me
know how I can help.
--
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 PR #3658: URL: https://github.com/apache/calcite/pull/3658#issuecomment-1914826743 This PR could cause some problems. We had trouble with a statement like ``` SELECT FROM "TA" AS "A" INNER JOIN "TB" AS "B" ON "A"."APPLICATION_ID" = "B"."CHILD_APPLICATION_ID" INNER JOIN "TC" AS "C" ON "C"."APPLICATION_ID" = "B"."PARENT_APPLICATION_ID" ``` `"B"."CHILD_APPLICATION_ID"` was hidden from the additional SELECT. -- 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-1914753294 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3658) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [2 New issues](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.0% 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]
kramerul commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1469610198
##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +257,47 @@ 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;
+if (builder.select.getSelectList() == SqlNodeList.SINGLETON_STAR) {
+ oldSelectList = new ArrayList<>();
+ for (int i = 0; i < rightFieldNames.size(); i++) {
+oldSelectList.add(new SqlIdentifier(rightFieldNames.get(i), POS));
+ }
+} else {
+ oldSelectList =
ImmutableList.copyOf(builder.select.getSelectList().getList());
Review Comment:
I fixed the CheckerFramework finding.
--
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_r1469598374
##
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);
Review Comment:
getKeyTypeOrThrow(SqlTypeUtil.deriveType(callBinding, op0)) is map key
--
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]
JiajunBernoulli commented on code in PR #3655:
URL: https://github.com/apache/calcite/pull/3655#discussion_r1469592343
##
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);
Review Comment:
Is it map key?
--
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]
JiajunBernoulli commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1469580846
##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +257,47 @@ 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;
+if (builder.select.getSelectList() == SqlNodeList.SINGLETON_STAR) {
+ oldSelectList = new ArrayList<>();
+ for (int i = 0; i < rightFieldNames.size(); i++) {
+oldSelectList.add(new SqlIdentifier(rightFieldNames.get(i), POS));
+ }
+} else {
+ oldSelectList =
ImmutableList.copyOf(builder.select.getSelectList().getList());
Review Comment:
Please fix CI
--
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-1914674069 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3658) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [2 New issues](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) [97.9% 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-1914500576 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3658) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [2 New issues](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) [97.9% 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-1914337344 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3658) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [2 New issues](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) [100.0% 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
[PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]
kramerul opened a new pull request, #3658: URL: https://github.com/apache/calcite/pull/3658 (no 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
