[GitHub] [calcite] DonnyZone commented on pull request #3203: [CALCITE-5703] Reduce amount of generated runtime code
DonnyZone commented on PR #3203:
URL: https://github.com/apache/calcite/pull/3203#issuecomment-1563744642
> > Sorry for the late reply. I make some tests in my local environment. The
optimization for `BinaryExpression` seems to be incorrect. The code after
optimization throws compilation error.
> > ```
> > Boolean a = true || null;
> > Boolean b = null || true;
> > ```
>
> @DonnyZone thanks for reply
>
> I can`t obtain such a code after optimization ( can you show me how can i
make it ?
>
> final ParameterExpression x_ = Expressions.parameter(Object.class, "x");
BinaryExpression y1 = Expressions.orElse(Expressions.constant(true),
Expressions.constant(null)); DeclarationStatement exp0 =
Expressions.declare(Modifier.PUBLIC, x_, y1);
>
> assertEquals("{\n public Object x = true;\n}\n", optimize(exp0))
>
> after optimization shows: public Object x = true;
I just pick the code from your UT.
```
@Test void testOptimizeBinaryNullCasting() {
// (v ? null : "a") == 1
assertEquals("{\n return (v || null) == 1;\n}\n",
optimize(
Expressions.equal(
Expressions.makeBinary(ExpressionType.OrElse,
Expressions.parameter(boolean.class, "v"),
new ConstantExpression(String.class, null)),
ONE)));
}
```
By the way, is it necessary to apply `skipNullCast` for the
`BinaryExpression`?
--
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
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3214: [CALCITE-5608] Implement ASINH, ACOSH, ATANH functions
sonarcloud[bot] commented on PR #3214: URL: https://github.com/apache/calcite/pull/3214#issuecomment-1563741869 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3214) [](https://sonarcloud.io/project/issues?id=apache_calcite=3214=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3214=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3214=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3214=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3214=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3214=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3214=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3214=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3214=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3214=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3214=false=CODE_SMELL) [7 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3214=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3214=new_coverage=list) [95.3% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3214=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3214=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3214=new_duplicated_lines_density=list) -- 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
[GitHub] [calcite] herunkang2018 commented on pull request #3217: [CALCITE-5717] Prune Project's input if project has no InputRef and the input has 1 row
herunkang2018 commented on PR #3217: URL: https://github.com/apache/calcite/pull/3217#issuecomment-1563681195 The improvement looks good to me, I have another question, is there other case that can be pruned beside this PR's case? -- 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
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3209: [CALCITE-4334] LITERAL_AGG, an aggregate function that returns a constant value
sonarcloud[bot] commented on PR #3209: URL: https://github.com/apache/calcite/pull/3209#issuecomment-1563581686 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3209) [](https://sonarcloud.io/project/issues?id=apache_calcite=3209=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3209=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3209=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3209=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3209=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3209=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3209=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3209=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3209=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3209=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3209=false=CODE_SMELL) [35 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3209=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3209=new_coverage=list) [85.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3209=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3209=new_duplicated_lines_density=list) [2.4% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3209=new_duplicated_lines_density=list) -- 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
[GitHub] [calcite] mihaibudiu commented on pull request #3145: [CALCITE-5615] Program to run SQL Logic Tests for Calcite
mihaibudiu commented on PR #3145: URL: https://github.com/apache/calcite/pull/3145#issuecomment-1563493613 Currently the "execute" method from Main only returns an integer and writes all diagnostic to the supplied streams. We have to make it return the actual TestStatistics object. So this will require another release. -- 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
[GitHub] [calcite] julianhyde commented on pull request #3145: [CALCITE-5615] Program to run SQL Logic Tests for Calcite
julianhyde commented on PR #3145: URL: https://github.com/apache/calcite/pull/3145#issuecomment-1563491610 Rather than checking that the number of failures doesn't change, it's better to check that there are no failures other than the list of 'expected failures' You don't want the situation where you fix one test and introduce a new failure. It's much better to have a list of known failures rather than just a count. -- 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
[GitHub] [calcite] zabetak commented on a diff in pull request #3145: [CALCITE-5615] Program to run SQL Logic Tests for Calcite
zabetak commented on code in PR #3145:
URL: https://github.com/apache/calcite/pull/3145#discussion_r1205976037
##
plus/src/test/java/org/apache/calcite/slt/executors/CalciteExecutor.java:
##
@@ -0,0 +1,170 @@
+/*
+ * 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.slt.executors;
+
+import org.apache.calcite.adapter.jdbc.JdbcSchema;
+import org.apache.calcite.jdbc.CalciteConnection;
+import org.apache.calcite.schema.SchemaPlus;
+
+import net.hydromatic.sqllogictest.ISqlTestOperation;
+import net.hydromatic.sqllogictest.OptionsParser;
+import net.hydromatic.sqllogictest.SltSqlStatement;
+import net.hydromatic.sqllogictest.SltTestFile;
+import net.hydromatic.sqllogictest.SqlTestQuery;
+import net.hydromatic.sqllogictest.TestStatistics;
+import net.hydromatic.sqllogictest.executors.HsqldbExecutor;
+import net.hydromatic.sqllogictest.executors.JdbcExecutor;
+import net.hydromatic.sqllogictest.executors.SqlSltTestExecutor;
+
+import java.io.IOException;
+import java.security.NoSuchAlgorithmException;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.Set;
+import javax.sql.DataSource;
+
+/**
+ * Executor for SQL logic tests using Calcite's JDBC adapter.
+ */
+public class CalciteExecutor extends SqlSltTestExecutor {
Review Comment:
I cannot find them either so let's consider this resolved :D Maybe they
never came out of my mind!
--
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
[GitHub] [calcite] zabetak commented on a diff in pull request #3145: [CALCITE-5615] Program to run SQL Logic Tests for Calcite
zabetak commented on code in PR #3145:
URL: https://github.com/apache/calcite/pull/3145#discussion_r1205973836
##
plus/src/test/java/org/apache/calcite/slt/TestCalcite.java:
##
@@ -0,0 +1,86 @@
+/*
+ * 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.slt;
+
+import org.apache.calcite.slt.executors.CalciteExecutor;
+
+import net.hydromatic.sqllogictest.OptionsParser;
+
+import org.junit.jupiter.api.Test;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.PrintStream;
+import java.nio.charset.StandardCharsets;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.core.Is.is;
+
+/**
+ * Tests using sql-logic-test suite.
+ */
+public class TestCalcite {
Review Comment:
There is nothing preventing us from re-redirecting the streams to files and
not keep them in byte buffers. Hopefully by doing this we should avoid running
out of memory.
We don't necessarily need to assert that there are no failures but more like
that the number of failures doesn't change. Unless the number of failures goes
down which I guess is a good thing meaning that we fixed a bug.
As far as I understand, point 2 that I suggested above is hard to implement
at this stage given that there are no test files that pass completely so we can
definitely defer to another 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
[GitHub] [calcite] tanclary commented on a diff in pull request #3214: [CALCITE-5608] Implement ASINH, ACOSH, ATANH functions
tanclary commented on code in PR #3214:
URL: https://github.com/apache/calcite/pull/3214#discussion_r1205820685
##
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##
@@ -1897,6 +1930,29 @@ public static double atan2(double b0, double b1) {
return Math.atan2(b0, b1);
}
+ // ATANH
+ /** SQL ATANH operator applied to BigDecimal values. */
+ public static double atanh(BigDecimal b) {
+return atanh(b.doubleValue());
+ }
+
+ /** SQL ATANH operator applied to double values. */
+ public static double atanh(double b) {
Review Comment:
nit: could just check abs(b) <= 1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #3209: [CALCITE-4334] LITERAL_AGG, an aggregate function that returns a constant value
julianhyde commented on code in PR #3209:
URL: https://github.com/apache/calcite/pull/3209#discussion_r1205725017
##
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##
@@ -1688,6 +1692,27 @@ static class GroupingImplementor implements
AggImplementor {
}
}
+ /** Implementor for the {@code LITERAL_AGG} aggregate function. */
+ static class LiteralAggImplementor implements AggImplementor {
+@Override public List getStateType(AggContext info) {
+ return ImmutableList.of();
+}
+
+@Override public void implementReset(AggContext info, AggResetContext
reset) {
+}
+
+@Override public void implementAdd(AggContext info, AggAddContext add) {
+}
+
+@Override public Expression implementResult(AggContext info,
+AggResultContext result) {
+ checkArgument(info.aggregation().kind == SqlKind.LITERAL_AGG);
+ checkArgument(result.call().rexList.size() == 1);
+ final RexNode rexNode = result.call().rexList.get(0);
Review Comment:
I don't feel strongly about it. There's some benefit to retaining
flexibility.
For example, in `Sort` we didn't validate that `limit` and `offset` were
literals and it turned out to be useful - because people wanted to use
parameters.
Clearly the expression should not contain `RexInputRef`. But calls to
parameter-less functions such as `CURRENT_DATE` seem like reasonable use cases.
--
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
[GitHub] [calcite] rubenada commented on a diff in pull request #3209: [CALCITE-4334] LITERAL_AGG, an aggregate function that returns a constant value
rubenada commented on code in PR #3209:
URL: https://github.com/apache/calcite/pull/3209#discussion_r1205653893
##
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##
@@ -1688,6 +1692,27 @@ static class GroupingImplementor implements
AggImplementor {
}
}
+ /** Implementor for the {@code LITERAL_AGG} aggregate function. */
+ static class LiteralAggImplementor implements AggImplementor {
+@Override public List getStateType(AggContext info) {
+ return ImmutableList.of();
+}
+
+@Override public void implementReset(AggContext info, AggResetContext
reset) {
+}
+
+@Override public void implementAdd(AggContext info, AggAddContext add) {
+}
+
+@Override public Expression implementResult(AggContext info,
+AggResultContext result) {
+ checkArgument(info.aggregation().kind == SqlKind.LITERAL_AGG);
+ checkArgument(result.call().rexList.size() == 1);
+ final RexNode rexNode = result.call().rexList.get(0);
Review Comment:
Should we check here that this rexNode is a RexLiteral?
--
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
[GitHub] [calcite] rubenada commented on pull request #3214: [CALCITE-5608] Implement ASINH, ACOSH, ATANH functions
rubenada commented on PR #3214: URL: https://github.com/apache/calcite/pull/3214#issuecomment-1562918294 @tanclary I see you have participated on other trigonometric-related PRs, would you like to take a look at this one? I think it is in a good shape and I will proceed to merge (after squashing commits) -- 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
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3225: [CALCITE-5723] Oracle dialect generates SQL that cannot be recognized by lower version Oracle Server(<12) when unparsing OffsetFetch
sonarcloud[bot] commented on PR #3225: URL: https://github.com/apache/calcite/pull/3225#issuecomment-1562862638 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3225) [](https://sonarcloud.io/project/issues?id=apache_calcite=3225=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3225=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3225=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3225=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3225=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3225=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3225=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3225=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3225=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3225=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3225=false=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_calcite=3225=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3225=new_coverage=list) [88.9% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3225=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3225=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3225=new_duplicated_lines_density=list) -- 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
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3223: [CALCITE-5721] Capture build scans on ge.apache.org to benefit from deep build insights
sonarcloud[bot] commented on PR #3223: URL: https://github.com/apache/calcite/pull/3223#issuecomment-1562850332 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3223) [](https://sonarcloud.io/project/issues?id=apache_calcite=3223=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3223=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3223=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3223=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3223=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3223=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3223=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3223=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3223=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3223=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3223=false=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3223=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3223=coverage=list) No Coverage information [](https://sonarcloud.io/component_measures?id=apache_calcite=3223=duplicated_lines_density=list) No Duplication information -- 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
[GitHub] [calcite] ILuffZhe opened a new pull request, #3225: [CALCITE-5723] Oracle dialect generates SQL that cannot be recognized by lower version Oracle Server(<12) when unparsing OffsetFetch
ILuffZhe opened a new pull request, #3225: URL: https://github.com/apache/calcite/pull/3225 (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
[GitHub] [calcite] clayburn commented on pull request #3223: [CALCITE-5721] Capture build scans on ge.apache.org to benefit from deep build insights
clayburn commented on PR #3223: URL: https://github.com/apache/calcite/pull/3223#issuecomment-1562836473 > Overall LTGM. Is there a way to test that it works as expected before merging? Sure, a few things you can do: * These are the two builds that have run so far for this PR in Jenkins: https://ge.apache.org/scans?search.rootProjectNames=calcite=America/Chicago * The GitHub Actions workflows aren't able to authenticate since they came from my fork, but you could create a branch in the `apache/calcite` repo and run the validations against that to see the resulting build scans. * You could run some local builds by authenticating to https://ge.apache.org with your ASF LDAP credentials, [provisioning an access token](https://docs.gradle.com/enterprise/gradle-plugin/#automated_access_key_provisioning), and generating scans from local builds. -- 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
[GitHub] [calcite] clayburn commented on a diff in pull request #3223: [CALCITE-5721] Capture build scans on ge.apache.org to benefit from deep build insights
clayburn commented on code in PR #3223:
URL: https://github.com/apache/calcite/pull/3223#discussion_r1205453958
##
settings.gradle.kts:
##
@@ -51,7 +53,8 @@ pluginManagement {
}
plugins {
-`gradle-enterprise`
+id("com.gradle.enterprise") version "3.13.2"
+id("com.gradle.common-custom-user-data-gradle-plugin") version "1.10"
Review Comment:
Done. Since this is a settings plugin, it still needs to be applied here,
but the versioning is not handled as you handle other plugins.
--
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
[GitHub] [calcite] zstan commented on pull request #3203: [CALCITE-5703] Reduce amount of generated runtime code
zstan commented on PR #3203:
URL: https://github.com/apache/calcite/pull/3203#issuecomment-1562446539
> Sorry for the late reply. I make some tests in my local environment. The
optimization for `BinaryExpression` seems to be incorrect. The code after
optimization throws compilation error.
>
> ```
> Boolean a = true || null;
> Boolean b = null || true;
> ```
@DonnyZone thanks for reply
I can\`t obtain such a code after optimization ( can you show me how can i
make it ?
final ParameterExpression x_ = Expressions.parameter(Object.class, "x");
BinaryExpression y1 = Expressions.orElse(Expressions.constant(true),
Expressions.constant(null));
DeclarationStatement exp0 = Expressions.declare(Modifier.PUBLIC, x_, y1);
assertEquals("{\n public Object x = true;\n}\n",
optimize(exp0))
after optimization shows:
public Object x = true;
--
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
[GitHub] [calcite] zabetak commented on a diff in pull request #3223: [CALCITE-5721] Capture build scans on ge.apache.org to benefit from deep build insights
zabetak commented on code in PR #3223:
URL: https://github.com/apache/calcite/pull/3223#discussion_r1205101753
##
settings.gradle.kts:
##
@@ -51,7 +53,8 @@ pluginManagement {
}
plugins {
-`gradle-enterprise`
+id("com.gradle.enterprise") version "3.13.2"
+id("com.gradle.common-custom-user-data-gradle-plugin") version "1.10"
Review Comment:
Usually we keep versions centralized in `gradle.properties` file. Is it
possible to use the `pluginManagement` section and the `idv` function to handle
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
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1205057374
##
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##
@@ -6823,6 +6823,48 @@ private void checkSemiJoinRuleOnAntiJoin(RelOptRule
rule) {
.checkUnchanged();
}
+ /** Test case for CALCITE-5683 for two level nested decorrelate with
standard program
+ * failing during the decorrelation phase. */
+ @Test void testTwoLevelDecorrelate() {
+final String sql = "SELECT d1.name, d1.deptno + "
++ " ( SELECT e1.empno "
++ " FROM emp e1 "
++ " WHERE d1.deptno = e1.deptno and "
++ " e1.sal = (SELECT max(sal) "
++ " FROM emp e2 "
++ " WHERE e1.sal = e2.sal and"
++ " e1.deptno = e2.deptno and"
++ " d1.deptno < e2.deptno))"
++ " FROM dept d1";
+
+sql(sql)
+.withExpand(false)
+.withLateDecorrelate(true)
+.withTrim(true)
+.withRule()
+.checkUnchanged();
Review Comment:
Yeah, please let me know if it makes sense to remove these tests (as they
actually are redundant) as sub-query.iq is having these test cases and are
reporting an error without the 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
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193: URL: https://github.com/apache/calcite/pull/3193#discussion_r1205054838 ## core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java: ## @@ -866,6 +866,8 @@ private static void matchFilter(SubQueryRemoveRule rule, LogicVisitor.find(RelOptUtil.Logic.TRUE, ImmutableList.of(c), e); final Set variablesSet = RelOptUtil.getVariablesUsed(e.rel); + /* Only consider the correlated variables which originated from this sub-query level*/ + variablesSet.retainAll(filter.getVariablesSet()); Review Comment: Agree that this fix is using the existing mechanism to set the variable in Filter operator, I did have questions like what happens when there other operator nodes etc. I want to handle the specific case for this PR(as you mentioned going in the right direction) and handle as we have more use cases (please let me know if it makes sense). Agree with you that this code might result in a throwing an exception in cases where previous code was generating a plan. However, I encountered a scenario while debugging the JIRA([CALCITE-5716](https://issues.apache.org/jira/browse/CALCITE-5716)) wherein it was not reporting an error but generating a wrong plan. With the fix provided in this PR it is generating a right result (when running a generated plan). Updated the JIRA with the analysis -- 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
[GitHub] [calcite] zoudan commented on a diff in pull request #3224: [CALCITE-5722] Fix RangeSets.isPoint to use Comparable equality
zoudan commented on code in PR #3224:
URL: https://github.com/apache/calcite/pull/3224#discussion_r1205054013
##
core/src/main/java/org/apache/calcite/util/RangeSets.java:
##
@@ -129,7 +129,7 @@ public static > int
hashCode(RangeSet rangeSet) {
public static > boolean isPoint(Range range) {
return range.hasLowerBound()
&& range.hasUpperBound()
-&& range.lowerEndpoint().equals(range.upperEndpoint())
+&& range.lowerEndpoint().compareTo(range.upperEndpoint()) == 0
Review Comment:
Maybe it is better to add comment for why we use `compareTo` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] zoudan commented on a diff in pull request #3224: [CALCITE-5722] Fix RangeSets.isPoint to use Comparable equality
zoudan commented on code in PR #3224:
URL: https://github.com/apache/calcite/pull/3224#discussion_r1205051820
##
core/src/test/java/org/apache/calcite/util/RangeSetTest.java:
##
@@ -155,6 +155,9 @@ class RangeSetTest {
assertThat(RangeSets.isPoint(Range.atMost(0)), is(false));
assertThat(RangeSets.isPoint(Range.greaterThan(0)), is(false));
assertThat(RangeSets.isPoint(Range.atLeast(0)), is(false));
+
+// Test situation where endpoints of closed range are equal under
`Comparable.compareTo` but not `T.equals`
Review Comment:
line too long, please check the code style
--
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
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
sonarcloud[bot] commented on PR #3193: URL: https://github.com/apache/calcite/pull/3193#issuecomment-1562349622 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3193) [](https://sonarcloud.io/project/issues?id=apache_calcite=3193=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3193=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3193=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3193=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3193=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3193=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3193=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3193=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3193=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3193=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3193=false=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3193=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3193=new_coverage=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3193=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3193=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3193=new_duplicated_lines_density=list) -- 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
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1205043890
##
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##
@@ -3549,7 +3549,17 @@ protected final void createAggImpl(
// implement HAVING (we have already checked that it is non-trivial)
relBuilder.push(bb.root());
if (havingExpr != null) {
- relBuilder.filter(havingExpr);
+ /* Set the correlation variables used in this sub-query to the filter
node,
+ * same logic is being used for the filter generated in where clause.
+ */
+ Set variableSet = new HashSet<>();
+ if (havingExpr instanceof RexSubQuery) {
Review Comment:
Agree that this might be taking care of limited cases, I didn't want to
increase the scope of this PR. Please let me know if you foresee any use cases
that is missing then I can take care of it in this PR (otherwise I think it
might be better to handle it in another PR).
--
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
