Re: [PR] [CALCITE-6162] Add rule(s) to remove joins with constant single tuple… [calcite]
HanumathRao commented on PR #3597: URL: https://github.com/apache/calcite/pull/3597#issuecomment-1894877595 > I have no further comments. Please squash the commits so we can merge this PR. Thank you for the improvements! Done. Thanks for review. -- 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-6199] Trim unused fields for SNAPSHOT and SAMPLE if table ha… [calcite]
libenchao commented on PR #3625: URL: https://github.com/apache/calcite/pull/3625#issuecomment-1894834942 @JiajunBernoulli I'm not sure the label "LGTM-will-merge-soon" should be added by the author. Generally the PRs should be reviewed by peers. To find a reviewer, you can ask for review in the Jira/PR/dev@ML. Fails all those approaches, and you are confident with your changes, you can commit it since you have commit privileges. -- 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-6200] RelJson throw UnsupportedOperationException for RexDyn… [calcite]
deathgripdez commented on PR #3626: URL: https://github.com/apache/calcite/pull/3626#issuecomment-1894821444 > ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3626) **Quality Gate passed** > The SonarCloud Quality Gate passed, but some issues were introduced. > > [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3626=false=true) > [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3626=false=true) > [92.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3626=new_coverage=list) > [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3626=new_duplicated_lines_density=list) > > [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3626) > > -- 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]
JiajunBernoulli commented on PR #3632: URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894809290 @jduo Yes, you can `squash` after any committer approve. But now we maybe need some new commits to make PR better. -- 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-6198] Add TO_TIMESTAMP_LTZ function (enabled in Snowflake li… [calcite]
JiajunBernoulli commented on PR #3631: URL: https://github.com/apache/calcite/pull/3631#issuecomment-1894805047 Does CI stability failed? https://github.com/apache/calcite/actions/runs/7549299627/attempts/1?pr=3631 -- 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-6207] Improve JDBC connection resource in JDBCUtils [calcite]
xuzifu666 commented on PR #3629: URL: https://github.com/apache/calcite/pull/3629#issuecomment-1894800473 > Apparently there is no way to reopen a merged PR, so please open a new one if there is an alternate fix. OK，I would open a new one -- 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-6205] Add BITAND_AGG, BITOR_AGG functions (enabled in Snowfl… [calcite]
JiajunBernoulli commented on code in PR #3630:
URL: https://github.com/apache/calcite/pull/3630#discussion_r1454328834
##
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##
@@ -6662,6 +6662,28 @@ private void checkLiteral2(String expression, String
expected) {
sql(sql).withMysql().ok(expectedMysql);
}
+ /** Test case for
+ * https://issues.apache.org/jira/browse/CALCITE-6205;>[CALCITE-6205]
+ * Add BITAND_AGG, BITOR_AGG functions (enabled in Snowflake library). */
+ @Test void testBitAndAgg() {
+final String query = "select bit_and(\"product_id\")\n"
++ "from \"product\"";
+final String expectedSnowflake = "SELECT BITAND_AGG(\"product_id\")\n"
++ "FROM \"foodmart\".\"product\"";
+
sql(query).withLibrary(SqlLibrary.SNOWFLAKE).withSnowflake().ok(expectedSnowflake);
+ }
+
+ /** Test case for
+ * https://issues.apache.org/jira/browse/CALCITE-6205;>[CALCITE-6205]
+ * Add BITAND_AGG, BITOR_AGG functions (enabled in Snowflake library). */
+ @Test void testBitOrAgg() {
+final String query = "select bit_or(\"product_id\")\n"
++ "from \"product\"";
+final String expectedSnowflake = "SELECT BITOR_AGG(\"product_id\")\n"
Review Comment:
Should we replace **BITOR_AGG** to **BITOR** when `unparse` BigQuery sql?
--
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-1894795481 Thanks @JiajunBernoulli. Do you mean I should squash my commits and give it the [CALCITE-5647] prefix? -- 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]
JiajunBernoulli commented on code in PR #3632:
URL: https://github.com/apache/calcite/pull/3632#discussion_r1454319255
##
core/src/test/java/org/apache/calcite/rel/metadata/RelMdPopulationSizeTest.java:
##
@@ -0,0 +1,80 @@
+/*
+ * 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.rel.metadata;
+
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.hep.HepPlanner;
+import org.apache.calcite.plan.hep.HepProgram;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.logical.LogicalValues;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeSystem;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.sql.type.SqlTypeFactoryImpl;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.util.ImmutableBitSet;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.math.BigDecimal;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests for {@link RelMdPopulationSize}.
+ */
+public class RelMdPopulationSizeTest {
+
+ @Test public void testPopulationSizeFromValues() {
Review Comment:
Can we add it in `RelMetadataTest`?
--
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]
JiajunBernoulli commented on PR #3632: URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894792002 @jduo Thanks for your PR. Just minor suggestion: 1. Commit message should start with `[CALCITE-5647]`. 2. We like adding Java Document and JIRA link before unit test. -- 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]
sonarcloud[bot] commented on PR #3632: URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894779707 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3632) **Quality Gate passed** Kudos, no new issues were introduced! [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3632=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3632=false=true) [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3632=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3632=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3632) -- 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]
sonarcloud[bot] commented on PR #3632: URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894756811 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3632) **Quality Gate passed** Kudos, no new issues were introduced! [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3632=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3632=false=true) [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3632=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3632=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3632) -- 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-1894742398 > There are some style violations Thanks. Should be correct now. -- 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-1894738317 There are some style violations -- 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 closed pull request #3151: CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) URL: https://github.com/apache/calcite/pull/3151 -- 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 #3151: URL: https://github.com/apache/calcite/pull/3151#issuecomment-1894737394 Closed in favor of https://github.com/apache/calcite/pull/3632 -- 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 #3151: URL: https://github.com/apache/calcite/pull/3151#issuecomment-1894736189 > Should this PR be closed? Yes, please do. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. 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 #3151: URL: https://github.com/apache/calcite/pull/3151#issuecomment-1894734973 Should this PR be closed? -- 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-1894734702 @libenchao @chunweilei -- 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 #3151: URL: https://github.com/apache/calcite/pull/3151#issuecomment-1894733709 Hi @adamkennedy, @libenchao and others. I'm continuing the work on this PR at #3632. (That PR has the unit test and is rebased on main). -- 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-6198] Add TO_TIMESTAMP_LTZ function (enabled in Snowflake li… [calcite]
tanclary opened a new pull request, #3631: URL: https://github.com/apache/calcite/pull/3631 …brary) -- 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-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
sonarcloud[bot] commented on PR #3522: URL: https://github.com/apache/calcite/pull/3522#issuecomment-1894706587 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3522) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3522=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3522=false=true) [97.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3522=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3522=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3522) -- 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-6162] Add rule(s) to remove joins with constant single tuple… [calcite]
mihaibudiu commented on PR #3597: URL: https://github.com/apache/calcite/pull/3597#issuecomment-1894661539 I have no further comments. Please squash the commits so we can merge this PR. Thank you for the improvements! -- 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-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
mihaibudiu commented on PR #3522: URL: https://github.com/apache/calcite/pull/3522#issuecomment-1894659736 > After @snuyanzin's comment I feel we need to get to the bottom of the test issue before merging, therefore marking the PR as "request changes" until clarified I have made the fixes you suggested. The tests should be run from the class CalciteSqlOperatorTest to exercise the evaluator. -- 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-6207] Improve JDBC connection resource in JDBCUtils [calcite]
mihaibudiu commented on PR #3629: URL: https://github.com/apache/calcite/pull/3629#issuecomment-1894650533 Apparently there is no way to reopen a merged PR, so please open a new one if there is an alternate fix. -- 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-6207] Improve JDBC connection resource in JDBCUtils [calcite]
mihaibudiu commented on PR #3629: URL: https://github.com/apache/calcite/pull/3629#issuecomment-1894649680 The discussion in the JIRA is about the behavior in the presence of exceptions. It seems that this improvement may cause a different exception to be thrown. -- 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-6207] Improve JDBC connection resource in JDBCUtils [calcite]
mihaibudiu commented on PR #3629: URL: https://github.com/apache/calcite/pull/3629#issuecomment-1894648920 I have merged, then cancelled the associated commit, since the change is still under debate in JIRA. -- 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 (8065ec7bde -> 86d5720c7f)
This is an automated email from the ASF dual-hosted git repository. mbudiu pushed a change to branch main in repository https://gitbox.apache.org/repos/asf/calcite.git omit 8065ec7bde [CALCITE-6207] Improve JDBC connection resource in JDBCUtils This update removed existing revisions from the reference, leaving the reference pointing at a previous point in the repository history. * -- * -- N refs/heads/main (86d5720c7f) \ O -- O -- O (8065ec7bde) Any revisions marked "omit" are not gone; other references still refer to them. Any revisions marked "discard" are gone forever. No new revisions were added by this update. Summary of changes: .../java/org/apache/calcite/adapter/jdbc/JdbcUtils.java| 14 +- 1 file changed, 13 insertions(+), 1 deletion(-)
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1454159011
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -13695,6 +13697,43 @@ private static void
checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * https://issues.apache.org/jira/browse/CALCITE-6111;>[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow.
*/
+ @Test public void testOverflow() {
+final SqlOperatorFixture f = fixture();
+f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)",
Byte.MAX_VALUE),
+".* out of range", true);
Review Comment:
If you call the test from SqlOperatorTest it doesn't involve the evaluation.
You have to call the test from CalciteSqlOperatorTest. That class extends
SqlOperatorTest but uses a test fixture which also evaluates the expressions.
In the IDE you can do this easiest if you paste the following in
CalciteSqlOperatorTest:
```
@Test
public void testOverflow() {
super.testOverflow();
}
```
--
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-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1454153342
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -657,10 +661,8 @@ void testCastExactNumericLimits(CastType castType,
SqlOperatorFixture f) {
f.checkCastToString(numeric.minNumericString, null, null, castType);
f.checkCastToString(numeric.minNumericString, type, null, castType);
- if (Bug.CALCITE_2539_FIXED) {
-f.checkCastFails("'notnumeric'", type, INVALID_CHAR_MESSAGE, true,
-castType);
- }
+ f.checkCastFails("'notnumeric'", type, INVALID_CHAR_MESSAGE, true,
Review Comment:
Yes, CALCITE-2539 seems to cover multiple issues, and I have fixed only one
of them here. There are other disabled tests guarded by this Bug.
--
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-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1454152204
##
linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java:
##
@@ -384,6 +384,10 @@ static void checkRoundedRange(Number value, double min,
double max) {
}
}
+ public static @Nullable Object integerCast(Primitive primitive, final Object
value) {
Review Comment:
Not quite reflection. The way Calcite evaluates constant expressions is to
generate a Java class with a Java method which is then compiled, loaded and
executed at runtime.
--
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-6207] Improve JDBC connection resource in JDBCUtils
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 8065ec7bde [CALCITE-6207] Improve JDBC connection resource in JDBCUtils
8065ec7bde is described below
commit 8065ec7bde557b2734fbdb44acdea22ea9e1b337
Author: xuyu <11161...@vivo.com>
AuthorDate: Tue Jan 16 16:24:21 2024 +0800
[CALCITE-6207] Improve JDBC connection resource in JDBCUtils
---
.../java/org/apache/calcite/adapter/jdbc/JdbcUtils.java| 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcUtils.java
b/core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcUtils.java
index ef48b505b5..493fc0335a 100644
--- a/core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcUtils.java
+++ b/core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcUtils.java
@@ -89,24 +89,12 @@ final class JdbcUtils {
Pair key) {
SqlDialectFactory dialectFactory = key.left;
DataSource dataSource = key.right;
- Connection connection = null;
- try {
-connection = dataSource.getConnection();
+ try (Connection connection = dataSource.getConnection()) {
DatabaseMetaData metaData = connection.getMetaData();
SqlDialect dialect = dialectFactory.create(metaData);
-connection.close();
-connection = null;
return dialect;
} catch (SQLException e) {
throw new RuntimeException(e);
- } finally {
-if (connection != null) {
- try {
-connection.close();
- } catch (SQLException e) {
-// ignore
- }
-}
}
}
Re: [PR] [CALCITE-6207] Improve JDBC connection resource in JDBCUtils [calcite]
mihaibudiu merged PR #3629: URL: https://github.com/apache/calcite/pull/3629 -- 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-6205] Add BITAND_AGG, BITOR_AGG functions (enabled in Snowfl… [calcite]
tanclary opened a new pull request, #3630: URL: https://github.com/apache/calcite/pull/3630 …ake library) -- 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-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453291779
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -13695,6 +13697,43 @@ private static void
checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * https://issues.apache.org/jira/browse/CALCITE-6111;>[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow.
*/
+ @Test public void testOverflow() {
+final SqlOperatorFixture f = fixture();
+f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)",
Byte.MAX_VALUE),
+".* out of range", true);
Review Comment:
I was expecting these tests to exercise the new code added to
`RexToLixTranslator`, but this doesn't seem to be the case (a breakpoint there
isn't hit when debugging such tests).
If that's a wrong assumption, what tests are covering the new code added to
`RexToLixTranslator`, and what part of the code is specifically tested by above
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-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453283126
##
linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java:
##
@@ -384,6 +384,10 @@ static void checkRoundedRange(Number value, double min,
double max) {
}
}
+ public static @Nullable Object integerCast(Primitive primitive, final Object
value) {
Review Comment:
Q: Is this used via reflection? My IDE suggests that the method is unused.
--
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-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453272159
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -13695,6 +13697,43 @@ private static void
checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * https://issues.apache.org/jira/browse/CALCITE-6111;>[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow.
*/
+ @Test public void testOverflow() {
+final SqlOperatorFixture f = fixture();
+f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)",
Byte.MAX_VALUE),
+".* out of range", true);
Review Comment:
@snuyanzin, I think it should fail as you say, there seems to be something
wrong.
I also tried changing `runtime` to `false` just to see, in this way it
always fails (with a cast with overflow, and cast without), I guess there is
something broken in the test auxiliary methods we rely on here.
The source of the problem lies outside the present PR, but it's nonetheless
a blocker for merging (if confirmed), as we can't rely on the unit tests.
Maybe @mihaibudiu has more familiarity with this part of the code, and can
shed some light.
--
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-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453272159
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -13695,6 +13697,43 @@ private static void
checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * https://issues.apache.org/jira/browse/CALCITE-6111;>[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow.
*/
+ @Test public void testOverflow() {
+final SqlOperatorFixture f = fixture();
+f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)",
Byte.MAX_VALUE),
+".* out of range", true);
Review Comment:
@snuyanzin, I think it should fail as you say, there seems to be something
wrong.
I also tried changing `runtime` to `false` just to see, in this way it
always fails (with a cast with overflow, and cast without, in both cases
parsing succeeds, so the check fail throws an error), I guess there is
something broken in the test auxiliary methods we rely on here.
The source of the problem lies outside the present PR, but it's nonetheless
a blocker for merging (if confirmed), as we can't rely on the unit tests.
Maybe @mihaibudiu has more familiarity with this part of the code, and can
shed some light.
--
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-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
snuyanzin commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453252536
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -13695,6 +13697,43 @@ private static void
checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * https://issues.apache.org/jira/browse/CALCITE-6111;>[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow.
*/
+ @Test public void testOverflow() {
+final SqlOperatorFixture f = fixture();
+f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)",
Byte.MAX_VALUE),
+".* out of range", true);
Review Comment:
I tried to see what happens if I locally change expectedError to something
else...
Surprisingly the tests continue passing
it seems that the reason is that this method is called
https://github.com/apache/calcite/blob/0bec957071468a2e54a22519290ac101a752fcad/testkit/src/main/java/org/apache/calcite/test/SqlOperatorFixtureImpl.java#L157-L171
which means it checks for failure only if it is not runtime...
so it could be any expected message and any number under `cast` and it
continues passing...
Or did I do something 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-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453189554
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -643,11 +646,12 @@ void testCastExactNumericLimits(CastType castType,
SqlOperatorFixture f) {
f.checkCastToScalarOkay("'" + numeric.minNumericString + "'",
type, numeric.minNumericString, castType);
- if (Bug.CALCITE_2539_FIXED) {
+ if (numeric != Numeric.DECIMAL5_2) {
+// The above conditoin is for bug CALCITE-6078
f.checkCastFails("'" + numeric.maxOverflowNumericString + "'",
-type, OUT_OF_RANGE_MESSAGE, true, castType);
+type, "Number has wrong format.*", true, castType);
Review Comment:
Nit: maybe we can introduce a variable `WRONG_FORMAT_MESSAGE` in the same
spirit of `OUT_OF_RANGE_MESSAGE` and others?
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -643,11 +646,12 @@ void testCastExactNumericLimits(CastType castType,
SqlOperatorFixture f) {
f.checkCastToScalarOkay("'" + numeric.minNumericString + "'",
type, numeric.minNumericString, castType);
- if (Bug.CALCITE_2539_FIXED) {
+ if (numeric != Numeric.DECIMAL5_2) {
+// The above conditoin is for bug CALCITE-6078
Review Comment:
```suggestion
// This condition is for bug [CALCITE-6078], not yet fixed
```
fixing a typo, reusing the same comment as above
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -643,11 +646,12 @@ void testCastExactNumericLimits(CastType castType,
SqlOperatorFixture f) {
f.checkCastToScalarOkay("'" + numeric.minNumericString + "'",
type, numeric.minNumericString, castType);
- if (Bug.CALCITE_2539_FIXED) {
+ if (numeric != Numeric.DECIMAL5_2) {
+// The above conditoin is for bug CALCITE-6078
f.checkCastFails("'" + numeric.maxOverflowNumericString + "'",
-type, OUT_OF_RANGE_MESSAGE, true, castType);
+type, "Number has wrong format.*", true, castType);
f.checkCastFails("'" + numeric.minOverflowNumericString + "'",
-type, OUT_OF_RANGE_MESSAGE, true, castType);
+type, "Number has wrong format.*", true, castType);
Review Comment:
fix here too if the proposal above is retained
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -657,10 +661,8 @@ void testCastExactNumericLimits(CastType castType,
SqlOperatorFixture f) {
f.checkCastToString(numeric.minNumericString, null, null, castType);
f.checkCastToString(numeric.minNumericString, type, null, castType);
- if (Bug.CALCITE_2539_FIXED) {
-f.checkCastFails("'notnumeric'", type, INVALID_CHAR_MESSAGE, true,
-castType);
- }
+ f.checkCastFails("'notnumeric'", type, INVALID_CHAR_MESSAGE, true,
Review Comment:
Question: `CALCITE-2539` isn't currently fixed, I guess you just verified
that the test passes and so confirmed it was safe to remove the "guard", right?
--
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-6193] If a query has more than one subexpression that matche… [calcite]
l4wei commented on code in PR #3624: URL: https://github.com/apache/calcite/pull/3624#discussion_r1453137778 ## core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java: ## @@ -575,9 +575,11 @@ assert rowTypesAreEquivalent( } } final MutableRel next = MutableRels.preOrderTraverseNext(queryDescendant); +final int currentInputIndex = queryDescendant.getInputs().size() > globalInputIndex +? globalInputIndex : 0; final MutableRel childOrNext = queryDescendant.getInputs().isEmpty() -? next : queryDescendant.getInputs().get(0); +? next : queryDescendant.getInputs().get(currentInputIndex); Review Comment: I think get(0) is correct, get(0) is just a starting point when traverse child-nodes, preOrderTraverseNext method can traverse the missing nodes. -- 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-6207] Improve JDBC connection resource in JDBCUtils [calcite]
sonarcloud[bot] commented on PR #3629: URL: https://github.com/apache/calcite/pull/3629#issuecomment-1893298949 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3629) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite=3629=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3629=false=true) [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3629=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3629=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3629) -- 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-6207] Improve connection resource in JDBCUtils [calcite]
xuzifu666 closed pull request #3628: [CALCITE-6207] Improve connection resource in JDBCUtils URL: https://github.com/apache/calcite/pull/3628 -- 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
