Re: [PR] [CALCITE-6038] Remove 'ORDER BY ... LIMIT n' when input has at most one row, n >= 1, and there is no 'OFFSET' clause [calcite]
LakeShen commented on code in PR #3467:
URL: https://github.com/apache/calcite/pull/3467#discussion_r1359139694
##
core/src/main/java/org/apache/calcite/rel/rules/CoreRules.java:
##
@@ -722,7 +722,7 @@ private CoreRules() {}
/** Rule that merge a {@link Sort} representing the Limit semantics and
* another {@link Sort} representing the Limit or TOPN semantics. */
- public static final SortMergeRule LIMIT_MREGE =
+ public static final SortMergeRule LIMIT_MERGE =
Review Comment:
Fix the Typo escaped
--
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-6038] Remove 'ORDER BY ... LIMIT n' when input has at most one row, n >= 1, and there is no 'OFFSET' clause [calcite]
LakeShen commented on PR #3467: URL: https://github.com/apache/calcite/pull/3467#issuecomment-1762586173 Hi @kgyrtkirk ,thank you for your review suggestions. I have modified according to your suggestions. If you have time, please help me review it again -- 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-6038] Remove 'ORDER BY ... LIMIT n' when input has at most one row, n >= 1, and there is no 'OFFSET' clause [calcite]
sonarcloud[bot] commented on PR #3467: URL: https://github.com/apache/calcite/pull/3467#issuecomment-1762547120 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3467) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_coverage=list) [94.7% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3467=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
Re: [PR] [CALCITE-6038] Remove 'ORDER BY ... LIMIT n' when input has at most one row, n >= 1, and there is no 'OFFSET' clause [calcite]
sonarcloud[bot] commented on PR #3467: URL: https://github.com/apache/calcite/pull/3467#issuecomment-1762517145 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3467) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_coverage=list) [94.1% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3467=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
Re: [PR] [CALCITE-6038] Remove 'ORDER BY ... LIMIT n' when input has at most one row, n >= 1, and there is no 'OFFSET' clause [calcite]
LakeShen commented on code in PR #3467:
URL: https://github.com/apache/calcite/pull/3467#discussion_r1359069846
##
core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRedundantRule.java:
##
@@ -97,13 +110,15 @@ protected SortRemoveRedundantRule(final
SortRemoveRedundantRule.Config config) {
}
private Optional getSortInputSpecificMaxRowCount(Sort sort) {
-// If the sort is pure limit, the specific max row count is limit's fetch.
-if (RelOptUtil.isPureLimit(sort)) {
- final double limit =
- sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
-1D;
- return Optional.of(limit);
+if (RelOptUtil.isLimit(sort)) {
+ final double fetch =
+ sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
0D;
+ if (fetch == 0D) {
+return Optional.empty();
+ }
+ // If sort is 'order by x limit n', the target max row count is 1.
+ return RelOptUtil.isOrder(sort) ? Optional.of(1D) : Optional.of(fetch);
} else if (RelOptUtil.isPureOrder(sort)) {
Review Comment:
`limit 0` would happen,and `RelOptUtil#isOrder` just checks whether there
is the sort field.
--
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-6038] Remove 'ORDER BY ... LIMIT n' when input has at most one row, n >= 1, and there is no 'OFFSET' clause [calcite]
sonarcloud[bot] commented on PR #3467: URL: https://github.com/apache/calcite/pull/3467#issuecomment-1762514894 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3467) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_coverage=list) [92.9% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3467=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
Re: [PR] [CALCITE-6038] Remove 'ORDER BY ... LIMIT n' when input has at most one row, n >= 1, and there is no 'OFFSET' clause [calcite]
LakeShen commented on code in PR #3467:
URL: https://github.com/apache/calcite/pull/3467#discussion_r1359069846
##
core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRedundantRule.java:
##
@@ -97,13 +110,15 @@ protected SortRemoveRedundantRule(final
SortRemoveRedundantRule.Config config) {
}
private Optional getSortInputSpecificMaxRowCount(Sort sort) {
-// If the sort is pure limit, the specific max row count is limit's fetch.
-if (RelOptUtil.isPureLimit(sort)) {
- final double limit =
- sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
-1D;
- return Optional.of(limit);
+if (RelOptUtil.isLimit(sort)) {
+ final double fetch =
+ sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
0D;
+ if (fetch == 0D) {
+return Optional.empty();
+ }
+ // If sort is 'order by x limit n', the target max row count is 1.
+ return RelOptUtil.isOrder(sort) ? Optional.of(1D) : Optional.of(fetch);
} else if (RelOptUtil.isPureOrder(sort)) {
Review Comment:
Get your point
--
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-6038] Remove 'ORDER BY ... LIMIT n' when input has at most one row, n >= 1, and there is no 'OFFSET' clause [calcite]
LakeShen commented on code in PR #3467:
URL: https://github.com/apache/calcite/pull/3467#discussion_r1359069805
##
core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRedundantRule.java:
##
@@ -97,13 +110,15 @@ protected SortRemoveRedundantRule(final
SortRemoveRedundantRule.Config config) {
}
private Optional getSortInputSpecificMaxRowCount(Sort sort) {
-// If the sort is pure limit, the specific max row count is limit's fetch.
-if (RelOptUtil.isPureLimit(sort)) {
- final double limit =
- sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
-1D;
- return Optional.of(limit);
+if (RelOptUtil.isLimit(sort)) {
+ final double fetch =
+ sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
0D;
+ if (fetch == 0D) {
+return Optional.empty();
+ }
+ // If sort is 'order by x limit n', the target max row count is 1.
+ return RelOptUtil.isOrder(sort) ? Optional.of(1D) : Optional.of(fetch);
Review Comment:
Fixed it with your advice
--
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-6038] Remove 'ORDER BY ... LIMIT n' when input has at most one row, n >= 1, and there is no 'OFFSET' clause [calcite]
LakeShen commented on code in PR #3467:
URL: https://github.com/apache/calcite/pull/3467#discussion_r1359064930
##
core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRedundantRule.java:
##
@@ -97,13 +110,15 @@ protected SortRemoveRedundantRule(final
SortRemoveRedundantRule.Config config) {
}
private Optional getSortInputSpecificMaxRowCount(Sort sort) {
-// If the sort is pure limit, the specific max row count is limit's fetch.
-if (RelOptUtil.isPureLimit(sort)) {
- final double limit =
- sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
-1D;
- return Optional.of(limit);
+if (RelOptUtil.isLimit(sort)) {
+ final double fetch =
Review Comment:
> I wonder why double is used around here as only `RexLiteral.intValue` is
used
You are right,Integer would be more appropriate,I will take this for your
advice.
--
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] 01/02: Code style: lint
This is an automated email from the ASF dual-hosted git repository.
jhyde pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/calcite.git
commit ce88348960e95e1c13da850b4422b2f49f022d93
Author: Julian Hyde
AuthorDate: Thu Jul 6 11:56:05 2023 -0700
Code style: lint
The next commit adds various lint rules. This commit fixes
the errors detected by those rules in existing code.
---
babel/src/test/resources/sql/big-query.iq | 12 +++-
.../calcite/adapter/enumerable/EnumUtils.java | 2 +-
.../adapter/enumerable/EnumerableLimitSort.java| 22 +-
.../calcite/adapter/enumerable/RexImpTable.java| 6 +++---
.../org/apache/calcite/interpreter/JoinNode.java | 6 +++---
.../apache/calcite/interpreter/UncollectNode.java | 2 +-
.../apache/calcite/jdbc/SimpleCalciteSchema.java | 2 +-
.../apache/calcite/plan/SubstitutionVisitor.java | 6 +++---
.../calcite/plan/volcano/VolcanoPlanner.java | 5 +++--
.../apache/calcite/rel/convert/ConverterRule.java | 5 +++--
.../calcite/rel/externalize/RelDotWriter.java | 7 +++
.../calcite/rel/hint/CompositeHintPredicate.java | 4 ++--
.../calcite/rel/metadata/RelMdColumnOrigins.java | 2 +-
.../rel/metadata/RelMdColumnUniqueness.java| 2 +-
.../calcite/rel/metadata/RelMdPredicates.java | 2 +-
.../calcite/rel/rel2sql/RelToSqlConverter.java | 6 +++---
.../apache/calcite/rel/rel2sql/SqlImplementor.java | 6 +++---
.../AggregateExpandDistinctAggregatesRule.java | 4 ++--
.../calcite/rel/rules/AggregateJoinRemoveRule.java | 5 +++--
.../AggregateProjectConstantToDummyJoinRule.java | 2 +-
.../calcite/rel/rules/MinusToDistinctRule.java | 5 +++--
.../rel/rules/ProjectJoinJoinRemoveRule.java | 2 +-
.../calcite/rel/rules/ProjectJoinRemoveRule.java | 2 +-
.../calcite/rel/type/RelDataTypeFactoryImpl.java | 4 ++--
.../java/org/apache/calcite/rex/RexProgram.java| 2 +-
.../org/apache/calcite/runtime/SqlFunctions.java | 4 ++--
.../org/apache/calcite/sql/SqlDataTypeSpec.java| 5 +++--
.../java/org/apache/calcite/sql/SqlIdentifier.java | 10 ++
.../calcite/sql/fun/SqlLibraryOperators.java | 2 +-
.../org/apache/calcite/sql/type/ReturnTypes.java | 2 +-
.../calcite/sql/validate/IdentifierNamespace.java | 5 +++--
.../calcite/sql/validate/SqlValidatorImpl.java | 2 +-
.../apache/calcite/sql2rel/RelDecorrelator.java| 12 +++-
.../apache/calcite/sql2rel/SqlToRelConverter.java | 2 +-
.../java/org/apache/calcite/tools/RelBuilder.java | 12 +++-
.../calcite/util/graph/DefaultDirectedGraph.java | 2 +-
.../org/apache/calcite/plan/RelWriterTest.java | 4 ++--
.../java/org/apache/calcite/sql/SqlNodeTest.java | 2 +-
.../calcite/test/MaterializedViewTester.java | 6 +++---
.../org/apache/calcite/test/MutableRelTest.java| 2 +-
.../java/org/apache/calcite/util/SourceTest.java | 2 +-
core/src/test/resources/sql/cast-with-format.iq| 1 +
.../apache/calcite/adapter/druid/DruidTable.java | 11 +++
.../adapter/elasticsearch/ElasticsearchJson.java | 2 +-
.../adapter/elasticsearch/ElasticsearchMethod.java | 2 +-
.../elasticsearch/ElasticsearchProject.java| 4 ++--
.../adapter/elasticsearch/ElasticsearchTable.java | 8
.../elasticsearch/ElasticsearchTableScan.java | 2 +-
.../adapter/elasticsearch/QueryBuilders.java | 6 +++---
.../adapter/elasticsearch/AggregationTest.java | 2 +-
.../elasticsearch/EmbeddedElasticsearchNode.java | 2 +-
.../elasticsearch/EmbeddedElasticsearchPolicy.java | 4 ++--
.../adapter/elasticsearch/ScrollingTest.java | 2 +-
.../apache/calcite/test/ElasticsearchChecker.java | 4 ++--
.../apache/calcite/adapter/file/CsvEnumerator.java | 5 +++--
.../apache/calcite/adapter/file/FileSchema.java| 5 +++--
.../calcite/adapter/innodb/IndexCondition.java | 9 ++---
innodb/src/test/resources/README.md| 12 ++--
.../apache/calcite/linq4j/tree/TryStatement.java | 2 +-
.../calcite/adapter/mongodb/MongoTableScan.java| 5 +++--
.../java/org/apache/calcite/slt/SqlLogicTests.java | 4 ++--
.../apache/calcite/adapter/redis/RedisTable.java | 5 +++--
.../apache/calcite/server/ServerDdlExecutor.java | 5 +++--
site/_docs/innodb_adapter.md | 6 +++---
.../org/apache/calcite/test/MockDdlExecutor.java | 5 +++--
65 files changed, 170 insertions(+), 140 deletions(-)
diff --git a/babel/src/test/resources/sql/big-query.iq
b/babel/src/test/resources/sql/big-query.iq
index 10c8ed6b67..ba9d034515 100755
--- a/babel/src/test/resources/sql/big-query.iq
+++ b/babel/src/test/resources/sql/big-query.iq
@@ -686,8 +686,8 @@ SELECT SAFE_ADD(CAST('NaN' AS DOUBLE), CAST(3 as BIGINT))
as NaN_result;
#
# SAFE_DIVIDE(value1, value2)
#
-# Equivalent to the divide operator (/), but returns NULL if
[calcite] 02/02: Add various lint checks
This is an automated email from the ASF dual-hosted git repository.
jhyde pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 54e3faf0618c25a63b1c40c0ec3855ce0b842127
Author: Julian Hyde
AuthorDate: Wed Jun 14 12:15:35 2023 -0700
Add various lint checks
(The errors detected by these rules were fixed in the preceding commit.)
Add a lint check '// not followed by space'.
Add a lint rule that ':' in 'for' must be surrounded by spaces.
Add a lint rule disallowing '?' without ':' on an assignment
line. It Converts
v = b ? branch1
: branch2;
to
v =
b ? branch1
: branch2;
Apply lint rules to all text files, not just Java files.
Detect tabs and trailing spaces.
Add 'lint:skip' directive.
Exempt LintTest from lint rules, because it contains strings
that are designed to trigger lint rules.
Treat the lint test string as a Java file.
In Puffin, add method Line.matcher(regex).
---
build.gradle.kts | 5 +-
.../main/java/org/apache/calcite/util/Puffin.java | 17 +++--
.../java/org/apache/calcite/test/LintTest.java | 72 --
.../java/org/apache/calcite/util/TestUnsafe.java | 20 +-
4 files changed, 99 insertions(+), 15 deletions(-)
diff --git a/build.gradle.kts b/build.gradle.kts
index 4b78f37ed7..90ce625516 100644
--- a/build.gradle.kts
+++ b/build.gradle.kts
@@ -603,8 +603,9 @@ allprojects {
replaceRegex("Method parameter list should not end in
whitespace or newline", "(?[CALCITE-...] link styles: 1",
"])++CALCITE-\\d+[^>]++>\\s*+\\[?(CALCITE-\\d+)\\]?", "https://issues.apache.org/jira/browse/\$1\;>[\$1]")
diff --git a/core/src/main/java/org/apache/calcite/util/Puffin.java
b/core/src/main/java/org/apache/calcite/util/Puffin.java
index 8b5d6e286c..cf52a92d57 100644
--- a/core/src/main/java/org/apache/calcite/util/Puffin.java
+++ b/core/src/main/java/org/apache/calcite/util/Puffin.java
@@ -35,6 +35,7 @@ import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.function.Supplier;
+import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Stream;
@@ -171,6 +172,8 @@ public class Puffin {
/** Returns whether the current line matches {@code regex}. */
boolean matches(String regex);
+Matcher matcher(String regex);
+
/** Returns the text of the current line. */
String line();
@@ -248,14 +251,14 @@ public class Puffin {
return fnr;
}
-@Override public String filename() {
- return source().toString();
-}
-
@Override public boolean isLast() {
return last;
}
+@Override public String filename() {
+ return source().toString();
+}
+
@Override public Source source() {
return source;
}
@@ -272,8 +275,12 @@ public class Puffin {
return line.endsWith(suffix);
}
+@Override public Matcher matcher(String regex) {
+ return pattern(regex).matcher(line);
+}
+
@Override public boolean matches(String regex) {
- return pattern(regex).matcher(line).matches();
+ return matcher(regex).matches();
}
@Override public String line() {
diff --git a/core/src/test/java/org/apache/calcite/test/LintTest.java
b/core/src/test/java/org/apache/calcite/test/LintTest.java
index bb1119d298..591ccb0f61 100644
--- a/core/src/test/java/org/apache/calcite/test/LintTest.java
+++ b/core/src/test/java/org/apache/calcite/test/LintTest.java
@@ -55,6 +55,26 @@ class LintTest {
.add(line -> line.fnr() == 1,
line -> line.globalState().fileCount++)
+// Skip directive
+.add(line -> line.matches(".* lint:skip ([0-9]+).*"),
+line -> {
+ final Matcher matcher = line.matcher(".* lint:skip ([0-9]+).*");
+ if (matcher.matches()) {
+int n = Integer.parseInt(matcher.group(1));
+line.state().skipToLine = line.fnr() + n;
+ }
+})
+
+// Trailing space
+.add(line -> line.endsWith(" "),
+line -> line.state().message("Trailing space", line))
+
+// Tab
+.add(line -> line.contains("\t")
+&& !line.filename().endsWith(".txt")
+&& !skipping(line),
+line -> line.state().message("Tab", line))
+
// Comment without space
.add(line -> line.matches(".* //[^ ].*")
&& !line.source().fileOpt()
@@ -65,13 +85,19 @@ class LintTest {
&& !line.contains("//CHECKSTYLE"),
line -> line.state().message("'//' must be followed by ' '", line))
+// In 'for (int i : list)', colon must be surrounded by space.
+.add(line -> line.matches("^ *for \\(.*:.*")
+&&
[calcite] branch main updated (e8af93f322 -> 54e3faf061)
This is an automated email from the ASF dual-hosted git repository. jhyde pushed a change to branch main in repository https://gitbox.apache.org/repos/asf/calcite.git from e8af93f322 [CALCITE-5950] DEFAULT expression is ignored during INSERT new ce88348960 Code style: lint new 54e3faf061 Add various lint checks The 2 revisions listed above as "new" are entirely new to this repository and will be described in separate emails. The revisions listed as "add" were already present in the repository and have only been added to this reference. Summary of changes: babel/src/test/resources/sql/big-query.iq | 12 ++-- build.gradle.kts | 5 +- .../calcite/adapter/enumerable/EnumUtils.java | 2 +- .../adapter/enumerable/EnumerableLimitSort.java| 22 --- .../calcite/adapter/enumerable/RexImpTable.java| 6 +- .../org/apache/calcite/interpreter/JoinNode.java | 6 +- .../apache/calcite/interpreter/UncollectNode.java | 2 +- .../apache/calcite/jdbc/SimpleCalciteSchema.java | 2 +- .../apache/calcite/plan/SubstitutionVisitor.java | 6 +- .../calcite/plan/volcano/VolcanoPlanner.java | 5 +- .../apache/calcite/rel/convert/ConverterRule.java | 5 +- .../calcite/rel/externalize/RelDotWriter.java | 7 +-- .../calcite/rel/hint/CompositeHintPredicate.java | 4 +- .../calcite/rel/metadata/RelMdColumnOrigins.java | 2 +- .../rel/metadata/RelMdColumnUniqueness.java| 2 +- .../calcite/rel/metadata/RelMdPredicates.java | 2 +- .../calcite/rel/rel2sql/RelToSqlConverter.java | 6 +- .../apache/calcite/rel/rel2sql/SqlImplementor.java | 6 +- .../AggregateExpandDistinctAggregatesRule.java | 4 +- .../calcite/rel/rules/AggregateJoinRemoveRule.java | 5 +- .../AggregateProjectConstantToDummyJoinRule.java | 2 +- .../calcite/rel/rules/MinusToDistinctRule.java | 5 +- .../rel/rules/ProjectJoinJoinRemoveRule.java | 2 +- .../calcite/rel/rules/ProjectJoinRemoveRule.java | 2 +- .../calcite/rel/type/RelDataTypeFactoryImpl.java | 4 +- .../java/org/apache/calcite/rex/RexProgram.java| 2 +- .../org/apache/calcite/runtime/SqlFunctions.java | 4 +- .../org/apache/calcite/sql/SqlDataTypeSpec.java| 5 +- .../java/org/apache/calcite/sql/SqlIdentifier.java | 10 +-- .../calcite/sql/fun/SqlLibraryOperators.java | 2 +- .../org/apache/calcite/sql/type/ReturnTypes.java | 2 +- .../calcite/sql/validate/IdentifierNamespace.java | 5 +- .../calcite/sql/validate/SqlValidatorImpl.java | 2 +- .../apache/calcite/sql2rel/RelDecorrelator.java| 12 ++-- .../apache/calcite/sql2rel/SqlToRelConverter.java | 2 +- .../java/org/apache/calcite/tools/RelBuilder.java | 12 ++-- .../main/java/org/apache/calcite/util/Puffin.java | 17 +++-- .../calcite/util/graph/DefaultDirectedGraph.java | 2 +- .../org/apache/calcite/plan/RelWriterTest.java | 4 +- .../java/org/apache/calcite/sql/SqlNodeTest.java | 2 +- .../java/org/apache/calcite/test/LintTest.java | 72 -- .../calcite/test/MaterializedViewTester.java | 6 +- .../org/apache/calcite/test/MutableRelTest.java| 2 +- .../java/org/apache/calcite/util/SourceTest.java | 2 +- .../java/org/apache/calcite/util/TestUnsafe.java | 20 +- core/src/test/resources/sql/cast-with-format.iq| 1 + .../apache/calcite/adapter/druid/DruidTable.java | 11 ++-- .../adapter/elasticsearch/ElasticsearchJson.java | 2 +- .../adapter/elasticsearch/ElasticsearchMethod.java | 2 +- .../elasticsearch/ElasticsearchProject.java| 4 +- .../adapter/elasticsearch/ElasticsearchTable.java | 8 +-- .../elasticsearch/ElasticsearchTableScan.java | 2 +- .../adapter/elasticsearch/QueryBuilders.java | 6 +- .../adapter/elasticsearch/AggregationTest.java | 2 +- .../elasticsearch/EmbeddedElasticsearchNode.java | 2 +- .../elasticsearch/EmbeddedElasticsearchPolicy.java | 4 +- .../adapter/elasticsearch/ScrollingTest.java | 2 +- .../apache/calcite/test/ElasticsearchChecker.java | 4 +- .../apache/calcite/adapter/file/CsvEnumerator.java | 5 +- .../apache/calcite/adapter/file/FileSchema.java| 5 +- .../calcite/adapter/innodb/IndexCondition.java | 9 ++- innodb/src/test/resources/README.md| 12 ++-- .../apache/calcite/linq4j/tree/TryStatement.java | 2 +- .../calcite/adapter/mongodb/MongoTableScan.java| 5 +- .../java/org/apache/calcite/slt/SqlLogicTests.java | 4 +- .../apache/calcite/adapter/redis/RedisTable.java | 5 +- .../apache/calcite/server/ServerDdlExecutor.java | 5 +- site/_docs/innodb_adapter.md | 6 +- .../org/apache/calcite/test/MockDdlExecutor.java | 5 +- 69 files changed, 269 insertions(+), 155 deletions(-)
Re: [PR] [CALCITE-5950] Default column constraint is erroneously processed [calcite]
asfgit closed pull request #3393: [CALCITE-5950] Default column constraint is erroneously processed URL: https://github.com/apache/calcite/pull/3393 -- 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-5950] DEFAULT expression is ignored during INSERT
This is an automated email from the ASF dual-hosted git repository.
jhyde 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 e8af93f322 [CALCITE-5950] DEFAULT expression is ignored during INSERT
e8af93f322 is described below
commit e8af93f32258a58d7548502dbfaa7e6ffd0db4cd
Author: zstan
AuthorDate: Tue Aug 22 17:30:57 2023 +0300
[CALCITE-5950] DEFAULT expression is ignored during INSERT
Close apache/calcite#3393
---
.../main/java/org/apache/calcite/sql/SqlUtil.java | 11 +++
.../apache/calcite/sql2rel/SqlToRelConverter.java | 99 --
.../org/apache/calcite/test/ServerParserTest.java | 7 ++
server/src/test/resources/sql/table.iq | 48 +++
4 files changed, 158 insertions(+), 7 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/sql/SqlUtil.java
b/core/src/main/java/org/apache/calcite/sql/SqlUtil.java
index d5e0b7e419..539c476a78 100644
--- a/core/src/main/java/org/apache/calcite/sql/SqlUtil.java
+++ b/core/src/main/java/org/apache/calcite/sql/SqlUtil.java
@@ -1238,6 +1238,17 @@ public abstract class SqlUtil {
return containsCall(node, callPredicate);
}
+ /**
+ * Returns whether a given node contains a {@link SqlKind#DEFAULT}
expression.
+ *
+ * @param node AST tree
+ */
+ public static boolean containsDefault(SqlNode node) {
+final Predicate callPredicate = call ->
+call.getKind() == SqlKind.DEFAULT;
+return containsCall(node, callPredicate);
+ }
+
/**
* Returns whether an AST tree contains a call to an aggregate function.
*
diff --git
a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
index 45853db7e3..21d37ff115 100644
--- a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
+++ b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
@@ -194,6 +194,7 @@ import java.lang.reflect.Type;
import java.math.BigDecimal;
import java.util.ArrayDeque;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.BitSet;
import java.util.Collection;
import java.util.Collections;
@@ -215,6 +216,7 @@ import static
com.google.common.base.Preconditions.checkArgument;
import static org.apache.calcite.linq4j.Nullness.castNonNull;
import static org.apache.calcite.runtime.FlatLists.append;
+import static org.apache.calcite.sql.SqlUtil.containsDefault;
import static org.apache.calcite.sql.SqlUtil.containsIn;
import static org.apache.calcite.sql.SqlUtil.stripAs;
@@ -281,6 +283,10 @@ public class SqlToRelConverter {
*/
private final Deque datasetStack = new ArrayDeque<>();
+ /** Stack that contains the SqlInsert operator that is currently being
+ * processed. It is used used for resolving DEFAULT expressions. */
+ private final Deque callStack = new ArrayDeque<>();
+
/**
* Mapping of non-correlated sub-queries that have been converted to their
* equivalent constants. Used to avoid re-evaluating the sub-query if it's
@@ -3801,6 +3807,8 @@ public class SqlToRelConverter {
protected RelNode convertInsert(SqlInsert call) {
RelOptTable targetTable = getTargetTable(call);
+callStack.push(call);
+
final RelDataType targetRowType =
validator().getValidatedNodeType(call);
assert targetRowType != null;
@@ -3808,6 +3816,8 @@ public class SqlToRelConverter {
convertQueryRecursive(call.getSource(), true, targetRowType).project();
RelNode massagedRel = convertColumnList(call, sourceRel);
+callStack.pop();
+
return createModify(targetTable, massagedRel);
}
@@ -4744,8 +4754,13 @@ public class SqlToRelConverter {
}
/**
- * Converts a values clause (as in "INSERT INTO T(x,y) VALUES (1,2)") into a
- * relational expression.
+ * Converts a VALUES clause into a relational expression.
+ *
+ * Example:
+ * {@code
+ * INSERT INTO T(x, y)
+ * VALUES (1, 2), (2, DEFAULT)
+ * }
*
* @param bbBlackboard
* @param valuesCall to SQL VALUES operator
@@ -4769,15 +4784,57 @@ public class SqlToRelConverter {
return;
}
-for (SqlNode rowConstructor1 : values.getOperandList()) {
- SqlCall rowConstructor = (SqlCall) rowConstructor1;
+final SqlCall insertOp = callStack.peek();
+// resolve presence of default params
+boolean processDefaults = (insertOp != null) && containsDefault(values);
+
+if (targetRowType == null) {
+ RelDataType listType = validator().getValidatedNodeType(values);
+ targetRowType = SqlTypeUtil.promoteToRowType(typeFactory, listType,
null);
+}
+
+assert insertOp instanceof SqlInsert || !processDefaults
+: "operation: " + insertOp;
+
+final InitializerExpressionFactory initializerFactory;
+final RelOptTable targetTable;
+final int[] mapping;
+
Re: [PR] [CALCITE-6014] Create a SqlOperatorFixture that parses, unparses, and… [calcite]
sonarcloud[bot] commented on PR #3433: URL: https://github.com/apache/calcite/pull/3433#issuecomment-1762047113 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3433) [](https://sonarcloud.io/project/issues?id=apache_calcite=3433=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3433=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3433=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3433=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3433=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3433=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3433=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3433=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3433=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3433=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3433=false=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3433=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3433=coverage=list) No Coverage information [](https://sonarcloud.io/component_measures?id=apache_calcite=3433=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3433=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
Re: [PR] [CALCITE-6014] Create a SqlOperatorFixture that parses, unparses, and… [calcite]
mihaibudiu commented on PR #3433: URL: https://github.com/apache/calcite/pull/3433#issuecomment-1762027962 I hope I have addressed the comments. I have optimistically squashed the commits to make this ready to merge. -- 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-6014] Create a SqlOperatorFixture that parses, unparses, and… [calcite]
mihaibudiu commented on code in PR #3433:
URL: https://github.com/apache/calcite/pull/3433#discussion_r1358693571
##
core/src/test/java/org/apache/calcite/test/SqlOperatorUnparseTest.java:
##
@@ -0,0 +1,116 @@
+/*
+ * 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.test;
+
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.sql.test.SqlOperatorFixture;
+import org.apache.calcite.sql.test.SqlTestFactory;
+
+import org.junit.jupiter.api.Disabled;
+
+import java.util.function.Consumer;
+import java.util.function.UnaryOperator;
+
+/**
+ * Version of a SqlOperatorTest which first parses and unparses
+ * the test program before executing it. Although similar to
+ * {@link org.apache.calcite.sql.parser.SqlUnParserTest},
Review Comment:
I have added a link, but at least intellij complains about that class not
being accessible, so I added a SuppressWarnings below.
--
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-5570] Support nested map type for SqlDataTypeSpec [calcite]
MasseGuillaume commented on PR #3105: URL: https://github.com/apache/calcite/pull/3105#issuecomment-1761964808 When you unparse with RelToSqlConverter it fails with: ``` [info] java.lang.UnsupportedOperationException: Unsupported type when convertTypeToSpec: MAP [info] at org.apache.calcite.sql.type.SqlTypeUtil.convertTypeToSpec(SqlTypeUtil.java:1140) [info] at org.apache.calcite.sql.SqlDialect.getCastSpec(SqlDialect.java:848) [info] at io.narrative.nql.package$$anon$1.getCastSpec(package.scala:50) [info] at org.apache.calcite.rel.rel2sql.RelToSqlConverter.castNullType(RelToSqlConverter.java:486) [info] at org.apache.calcite.rel.rel2sql.RelToSqlConverter.visit(RelToSqlConverter.java:458) [info] at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) [info] at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) [info] at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) [info] at java.base/java.lang.reflect.Method.invoke(Method.java:568) [info] at org.apache.calcite.util.ReflectUtil$2.invoke(ReflectUtil.java:532) ``` I think you need to add a branch for the Map type in https://github.com/apache/calcite/blob/485a5d0ae3274a83a36788290b3bdc4d973387b4/core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java#L1109 -- 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-6014] Create a SqlOperatorFixture that parses, unparses, and… [calcite]
rubenada commented on code in PR #3433: URL: https://github.com/apache/calcite/pull/3433#discussion_r1358613591 ## core/src/test/java/org/apache/calcite/test/SqlOperatorUnparseTest.java: ## @@ -0,0 +1,114 @@ +/* + * 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.test; + +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.parser.SqlParseException; +import org.apache.calcite.sql.parser.SqlParser; +import org.apache.calcite.sql.test.SqlOperatorFixture; +import org.apache.calcite.sql.test.SqlTestFactory; + +import org.junit.jupiter.api.Disabled; + +import java.util.function.Consumer; +import java.util.function.UnaryOperator; + +/** + * Version of a SqlOperatorTest which first parses and unparses + * the test program before executing it. Although similar to Review Comment: Ok, no big deal, let's leave it like this. I just noticed that it is the same on the license comment at the beginning of the file. -- 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-5806] add a String data type (enabled in Apache Spark and BigQuery) [calcite]
MasseGuillaume commented on PR #3468:
URL: https://github.com/apache/calcite/pull/3468#issuecomment-1761932601
Yes `string` is a first class citizen.
```
scala> spark.sql("""select cast(NULL as string)""").show()
++
|CAST(NULL AS STRING)|
++
|null|
++
```
--
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-6014] Create a SqlOperatorFixture that parses, unparses, and… [calcite]
mihaibudiu commented on code in PR #3433: URL: https://github.com/apache/calcite/pull/3433#discussion_r1358599952 ## core/src/test/java/org/apache/calcite/test/SqlOperatorUnparseTest.java: ## @@ -0,0 +1,114 @@ +/* + * 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.test; + +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.parser.SqlParseException; +import org.apache.calcite.sql.parser.SqlParser; +import org.apache.calcite.sql.test.SqlOperatorFixture; +import org.apache.calcite.sql.test.SqlTestFactory; + +import org.junit.jupiter.api.Disabled; + +import java.util.function.Consumer; +import java.util.function.UnaryOperator; + +/** + * Version of a SqlOperatorTest which first parses and unparses + * the test program before executing it. Although similar to Review Comment: As a longtime emacs users I have been trained to use two spaces after the real periods. I can remove this, although I personally find it more readable. -- 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-6014] Create a SqlOperatorFixture that parses, unparses, and… [calcite]
rubenada commented on PR #3433: URL: https://github.com/apache/calcite/pull/3433#issuecomment-1761914996 Thanks @mihaibudiu for moving this forward, I think this is a good idea to unveil bugs. I think the PR is in a pretty good shape, I have just left some minor comments. -- 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-6014] Create a SqlOperatorFixture that parses, unparses, and… [calcite]
rubenada commented on code in PR #3433: URL: https://github.com/apache/calcite/pull/3433#discussion_r1358597615 ## core/src/test/java/org/apache/calcite/test/SqlOperatorUnparseTest.java: ## @@ -0,0 +1,114 @@ +/* + * 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.test; + +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.parser.SqlParseException; +import org.apache.calcite.sql.parser.SqlParser; +import org.apache.calcite.sql.test.SqlOperatorFixture; +import org.apache.calcite.sql.test.SqlTestFactory; + +import org.junit.jupiter.api.Disabled; + +import java.util.function.Consumer; +import java.util.function.UnaryOperator; + +/** + * Version of a SqlOperatorTest which first parses and unparses + * the test program before executing it. Although similar to Review Comment: Is it possible that there is an extra whitespace after the dot and "Although" ? -- 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-6014] Create a SqlOperatorFixture that parses, unparses, and… [calcite]
rubenada commented on code in PR #3433: URL: https://github.com/apache/calcite/pull/3433#discussion_r1358597123 ## core/src/test/java/org/apache/calcite/test/SqlOperatorUnparseTest.java: ## @@ -0,0 +1,114 @@ +/* + * 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.test; + +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.parser.SqlParseException; +import org.apache.calcite.sql.parser.SqlParser; +import org.apache.calcite.sql.test.SqlOperatorFixture; +import org.apache.calcite.sql.test.SqlTestFactory; + +import org.junit.jupiter.api.Disabled; + +import java.util.function.Consumer; +import java.util.function.UnaryOperator; + +/** + * Version of a SqlOperatorTest which first parses and unparses + * the test program before executing it. Although similar to + * SqlUnparserTest, this test also validates the code after unparsing. Review Comment: Perhaps adding a link to `SqlUnparserTest`? -- 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-5806] add a String data type (enabled in Apache Spark and BigQuery) [calcite]
mihaibudiu commented on PR #3468: URL: https://github.com/apache/calcite/pull/3468#issuecomment-176190 I just expressed my personal opinion above; my experience with Calcite isn't that significant, so I have been wrong before. Regarding SPARK, you should file an issue, and it looks like you can perhaps send a PR for the fix for it as well. Does the SPARK parser parse STRING types already? -- 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-5806] add a String data type (enabled in Apache Spark and BigQuery) [calcite]
MasseGuillaume closed pull request #3468: [CALCITE-5806] add a String data type (enabled in Apache Spark and BigQuery) URL: https://github.com/apache/calcite/pull/3468 -- 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-5806] add a String data type (enabled in Apache Spark and BigQuery) [calcite]
MasseGuillaume commented on PR #3468: URL: https://github.com/apache/calcite/pull/3468#issuecomment-1761903811 Okay, I will close this PR and write a how-to guide to defined custom types and use string as an example. However, there is a bug for the Spark SQL dialect in RelToSqlConverter since it does not unparse varchar to string. -- 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-5826] Add FIND_IN_SET function (enabled in Hive and Spark library) [calcite]
mihaibudiu commented on code in PR #3317: URL: https://github.com/apache/calcite/pull/3317#discussion_r1358584847 ## site/_docs/reference.md: ## @@ -2729,6 +2729,7 @@ BigQuery's type system uses confusingly different names for types and functions: | o | EXTRACT(xml, xpath, [, namespaces ]) | Returns the XML fragment of the element or elements matched by the XPath expression. The optional namespace value that specifies a default mapping or namespace mapping for prefixes, which is used when evaluating the XPath expression | o | EXISTSNODE(xml, xpath, [, namespaces ])| Determines whether traversal of a XML document using a specified xpath results in any nodes. Returns 0 if no nodes remain after applying the XPath traversal on the document fragment of the element or elements matched by the XPath expression. Returns 1 if any nodes remain. The optional namespace value that specifies a default mapping or namespace mapping for prefixes, which is used when evaluating the XPath expression. | m | EXTRACTVALUE(xml, xpathExpr)) | Returns the text of the first text node which is a child of the element or elements matched by the XPath expression. +| h s | FIND_IN_SET(string, stringArray) | Returns the index (1-based) of the given *string* in the comma-delimited *stringArray*. Returns 0, if the given *string* was not found or if *string* contains a comma Review Comment: From this description I infer that stringArray is of type `ARRAY STRING`, however it seems to be just a string which may contain commas. (I also think that the function name is not a very good choice, but that cannot be changed.) -- 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-6042] Add test cases for ARRAY-related functions by using spark array function (Spark Library) [calcite]
mihaibudiu commented on PR #3462: URL: https://github.com/apache/calcite/pull/3462#issuecomment-1761888767 There is actually an even better way to do it, I should have thought about it sooner. The idea is to use Calcite itself to rewrite the code. The way this is done is by implementing a SqlShuttle which replaces calls to one of the operators with calls to the other one. Then for each test you: - run the test as is - parse the Sql, run the shuttle, unparse the sql - run the resulting sql Let me know if you need help implementing this solution. -- 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-5806] add a String data type (enabled in Apache Spark and BigQuery) [calcite]
mihaibudiu commented on PR #3468: URL: https://github.com/apache/calcite/pull/3468#issuecomment-1761882077 Once you add a new type you have to maintain it forever. Moreover, technically you should write many more tests; everything that works with varchar should be tested with string if you want to be sure it works. If something can be done without modifying Calcite I think that's the right way to do it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-5806] add a String data type (enabled in Apache Spark and BigQuery) [calcite]
MasseGuillaume commented on PR #3468:
URL: https://github.com/apache/calcite/pull/3468#issuecomment-1761811485
Ah, to unparse user defined types it's via:
`SqlDialect.getCastSpec` then you can use `SqlUserDefinedTypeNameSpec`
```scala
val pos = SqlParserPos.ZERO
new SqlDataTypeSpec(new SqlUserDefinedTypeNameSpec(new
SqlIdentifier("STRING", pos), pos), pos)
```
While it's possible to add a type without modifying Calcite's codebase, it's
a lot of gymnastics for a type that is fairly common in practice:
https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#string_type
https://spark.apache.org/docs/latest/sql-ref-datatypes.html
--
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-5806] add a String data type (enabled in Apache Spark and BigQuery) [calcite]
sonarcloud[bot] commented on PR #3468: URL: https://github.com/apache/calcite/pull/3468#issuecomment-1761648018 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3468) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3468=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3468=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3468=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3468=new_coverage=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3468=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3468=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3468=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
Re: [PR] [CALCITE-5806] add a String data type (enabled in Apache Spark and BigQuery) [calcite]
sonarcloud[bot] commented on PR #3468: URL: https://github.com/apache/calcite/pull/3468#issuecomment-1761620267 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3468) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3468=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3468=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3468=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3468=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3468=new_coverage=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3468=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3468=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3468=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
Re: [PR] [CALCITE-5806] add a String data type (enabled in Apache Spark and BigQuery) [calcite]
MasseGuillaume commented on PR #3468:
URL: https://github.com/apache/calcite/pull/3468#issuecomment-1761572838
Good point, I also consider this alias.
While implementing I saw there is an extension for types:
```
parser.dataTypeParserMethods!default.parser.dataTypeParserMethods
```
For RelToSqlConverter, how would I solve unparse for VARCHAR for Apache
Spark?
```scala
scala> spark.sql("""select cast("foo" as VARCHAR)""").show()
org.apache.spark.sql.catalyst.parser.ParseException:
[DATATYPE_MISSING_SIZE] DataType "VARCHAR" requires a length parameter, for
example "VARCHAR"(10). Please specify the length.(line 1, pos 21)
== SQL ==
select cast("foo" as VARCHAR)
-^^^
```
--
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-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459:
URL: https://github.com/apache/calcite/pull/3459#discussion_r1358246934
##
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##
@@ -1082,6 +1084,38 @@ private static RelDataType
arrayReturnType(SqlOperatorBinding opBinding) {
SqlLibraryOperators::arrayReturnType,
OperandTypes.SAME_VARIADIC);
+ private static RelDataType mapReturnType(SqlOperatorBinding opBinding) {
+Pair<@Nullable RelDataType, @Nullable RelDataType> type =
+getComponentTypes(
+opBinding.getTypeFactory(), opBinding.collectOperandTypes());
+return SqlTypeUtil.createMapType(
+opBinding.getTypeFactory(),
+requireNonNull(type.left, "inferred key type"),
+requireNonNull(type.right, "inferred value type"),
+false);
+ }
+
+ private static Pair<@Nullable RelDataType, @Nullable RelDataType>
getComponentTypes(
+ RelDataTypeFactory typeFactory,
+ List argTypes) {
+// special case, allows empty map
+if (argTypes.size() == 0) {
Review Comment:
yes, fixed
--
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-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459:
URL: https://github.com/apache/calcite/pull/3459#discussion_r1358161626
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -6703,11 +6704,53 @@ private static void checkIf(SqlOperatorFixture f) {
// test operands not in same type family.
f.checkFails("^map_concat(map[1, null], array[1])^",
"Parameters must be of the same type", false);
+
+// 2. check with map function, map(k, v ...)
+final SqlOperatorFixture f1 = fixture()
+.setFor(SqlLibraryOperators.MAP_CONCAT)
+.withLibrary(SqlLibrary.SPARK);
+f1.checkScalar("map_concat(map('foo', 1), map('bar', 2))", "{foo=1,
bar=2}",
+"(CHAR(3) NOT NULL, INTEGER NOT NULL) MAP NOT NULL");
+f1.checkScalar("map_concat(map('foo', 1), map('bar', 2), map('foo', 2))",
"{foo=2, bar=2}",
+"(CHAR(3) NOT NULL, INTEGER NOT NULL) MAP NOT NULL");
+f1.checkScalar("map_concat(map(null, 1), map(null, 2))", "{null=2}",
+"(NULL, INTEGER NOT NULL) MAP NOT NULL");
+f1.checkScalar("map_concat(map(1, 2), map(1, null))", "{1=null}",
+"(INTEGER NOT NULL, INTEGER) MAP NOT NULL");
+// test zero arg, but it should return empty map.
+f1.checkScalar("map_concat()", "{}",
+"(VARCHAR NOT NULL, VARCHAR NOT NULL) MAP");
+
+// test concat with empty map, in fact, spark will return MAP(CHAR,
INTEGER), because
+// it will add a cast 'cast(map() as map)' to convert UNKNOWN
type,
+// but currently calcite not support cast to map type.
+f1.checkScalar("map_concat(map('foo', 1), map())", "{foo=1}",
+"(UNKNOWN NOT NULL, UNKNOWN NOT NULL) MAP NOT NULL");
+
+// after calcite supports cast(null as map), cast(map() as
map)
+// it should add these tests.
+if (TODO) {
Review Comment:
I get your point. but the form such as `cast(map() as map)` may
not necessarily be a bug, and there is currently no JIRA to track it, calcite
is not ready to support it yet, and may not support it, so it is kept as a TODO
for the time being. WDYT?
--
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-5918] Add MAP function (enabled in Spark library) [calcite]
sonarcloud[bot] commented on PR #3459: URL: https://github.com/apache/calcite/pull/3459#issuecomment-1761405658 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3459) [](https://sonarcloud.io/project/issues?id=apache_calcite=3459=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3459=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3459=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3459=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3459=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3459=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3459=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3459=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3459=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3459=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3459=false=CODE_SMELL) [16 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3459=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3459=new_coverage=list) [97.6% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3459=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3459=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3459=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
Re: [PR] [CALCITE-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on PR #3459: URL: https://github.com/apache/calcite/pull/3459#issuecomment-1761402938 hi, @tanclary I have addressed all your comments except `instead of if (TODO) would it be better to add an enum to the Bug class?` this one. I have rebased the main accidentally, I hope it won’t cause any trouble to your review. If you have time, pls help me to review it ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459: URL: https://github.com/apache/calcite/pull/3459#discussion_r1358166406 ## core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java: ## @@ -568,6 +569,9 @@ public static SqlOperandTypeChecker variadic( public static final SqlSingleOperandTypeChecker MAP_FROM_ENTRIES = new MapFromEntriesOperandTypeChecker(); + public static final SqlSingleOperandTypeChecker MAP_FUNCTION = Review Comment: fixed. -- 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-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459:
URL: https://github.com/apache/calcite/pull/3459#discussion_r1358166726
##
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##
@@ -1221,6 +1225,53 @@ private static class MapFromEntriesOperandTypeChecker
}
}
+ /**
+ * Operand type-checking strategy for a MAP function, it allows empty map.
+ */
+ private static class MapFunctionOperandTypeChecker
+ extends SameOperandTypeChecker {
+
+MapFunctionOperandTypeChecker() {
+ super(-1);
+}
+
+@Override public boolean checkOperandTypes(final SqlCallBinding
callBinding,
+final boolean throwOnFailure) {
+ final List argTypes =
+ SqlTypeUtil.deriveType(callBinding, callBinding.operands());
+ // allows empty map
+ if (argTypes.size() == 0) {
+return true;
+ }
+ // the size of map arg types must be even.
+ if (argTypes.size() % 2 > 0) {
Review Comment:
fixed.
--
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-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459:
URL: https://github.com/apache/calcite/pull/3459#discussion_r1358164236
##
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##
@@ -873,6 +874,7 @@ Builder populate2() {
map.put(MAP_VALUE_CONSTRUCTOR, value);
map.put(ARRAY_VALUE_CONSTRUCTOR, value);
defineMethod(ARRAY, BuiltInMethod.ARRAYS_AS_LIST.method,
NullPolicy.NONE);
+ defineMethod(MAP, BuiltInMethod.MAP_FUNCTION.method, NullPolicy.NONE);
Review Comment:
In fact, there is a MAP object in BuiltInMethod, occupy this name. but yes,
if we rename that, we can use MAP. I have updated it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459:
URL: https://github.com/apache/calcite/pull/3459#discussion_r1358164736
##
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##
@@ -5251,6 +5251,17 @@ public static Map mapFromArrays(List keysArray, List
valuesArray) {
return map;
}
+ /** Support the MAP function. */
+ public static Map mapFunction(Object... args) {
Review Comment:
fixed.
--
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-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459:
URL: https://github.com/apache/calcite/pull/3459#discussion_r1358164236
##
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##
@@ -873,6 +874,7 @@ Builder populate2() {
map.put(MAP_VALUE_CONSTRUCTOR, value);
map.put(ARRAY_VALUE_CONSTRUCTOR, value);
defineMethod(ARRAY, BuiltInMethod.ARRAYS_AS_LIST.method,
NullPolicy.NONE);
+ defineMethod(MAP, BuiltInMethod.MAP_FUNCTION.method, NullPolicy.NONE);
Review Comment:
In fact, there is a MAP object in BuiltInMethod, occupy this name. but yes,
if we rename that, we can use MAP. I have updated it. WDYT?
--
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-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459: URL: https://github.com/apache/calcite/pull/3459#discussion_r1358162190 ## site/_docs/reference.md: ## @@ -2769,6 +2769,7 @@ BigQuery's type system uses confusingly different names for types and functions: | b | TO_HEX(binary) | Converts *binary* into a hexadecimal varchar | b | FROM_HEX(varchar) | Converts a hexadecimal-encoded *varchar* into bytes | b o | LTRIM(string)| Returns *string* with all blanks removed from the start +| s | MAP(key, value [, key, value]*)| Returns a map with the given key/value pairs Review Comment: yes, fixed. -- 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-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459:
URL: https://github.com/apache/calcite/pull/3459#discussion_r1358161626
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -6703,11 +6704,53 @@ private static void checkIf(SqlOperatorFixture f) {
// test operands not in same type family.
f.checkFails("^map_concat(map[1, null], array[1])^",
"Parameters must be of the same type", false);
+
+// 2. check with map function, map(k, v ...)
+final SqlOperatorFixture f1 = fixture()
+.setFor(SqlLibraryOperators.MAP_CONCAT)
+.withLibrary(SqlLibrary.SPARK);
+f1.checkScalar("map_concat(map('foo', 1), map('bar', 2))", "{foo=1,
bar=2}",
+"(CHAR(3) NOT NULL, INTEGER NOT NULL) MAP NOT NULL");
+f1.checkScalar("map_concat(map('foo', 1), map('bar', 2), map('foo', 2))",
"{foo=2, bar=2}",
+"(CHAR(3) NOT NULL, INTEGER NOT NULL) MAP NOT NULL");
+f1.checkScalar("map_concat(map(null, 1), map(null, 2))", "{null=2}",
+"(NULL, INTEGER NOT NULL) MAP NOT NULL");
+f1.checkScalar("map_concat(map(1, 2), map(1, null))", "{1=null}",
+"(INTEGER NOT NULL, INTEGER) MAP NOT NULL");
+// test zero arg, but it should return empty map.
+f1.checkScalar("map_concat()", "{}",
+"(VARCHAR NOT NULL, VARCHAR NOT NULL) MAP");
+
+// test concat with empty map, in fact, spark will return MAP(CHAR,
INTEGER), because
+// it will add a cast 'cast(map() as map)' to convert UNKNOWN
type,
+// but currently calcite not support cast to map type.
+f1.checkScalar("map_concat(map('foo', 1), map())", "{foo=1}",
+"(UNKNOWN NOT NULL, UNKNOWN NOT NULL) MAP NOT NULL");
+
+// after calcite supports cast(null as map), cast(map() as
map)
+// it should add these tests.
+if (TODO) {
Review Comment:
I get your point. but the form such as `cast(map() as map)` may
not necessarily be a bug, and there is currently no JIRA to track it, calcite
is not ready to support it yet, and may not support it, so it is kept as a TODO
for the time being.
--
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-6022] Support "CREATE TABLE LIKE" DDL [calcite]
JiajunBernoulli commented on code in PR #3442: URL: https://github.com/apache/calcite/pull/3442#discussion_r1358158732 ## core/src/main/codegen/templates/Parser.jj: ## @@ -4318,8 +4318,13 @@ SqlCreate SqlCreate() : ( <#-- additional literal parser methods are included here --> <#list (parser.createStatementParserMethods!default.parser.createStatementParserMethods) as method> +<#if method = "SqlCreateTableLike"> Review Comment: Sorry, I misled you. Please drop the last commit(Add SqlCreateTableLike to createStatementParserMethods).  I will merge it at the weekends. -- 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-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459:
URL: https://github.com/apache/calcite/pull/3459#discussion_r1358157015
##
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##
@@ -1221,6 +1225,53 @@ private static class MapFromEntriesOperandTypeChecker
}
}
+ /**
+ * Operand type-checking strategy for a MAP function, it allows empty map.
+ */
+ private static class MapFunctionOperandTypeChecker
+ extends SameOperandTypeChecker {
+
+MapFunctionOperandTypeChecker() {
+ super(-1);
Review Comment:
The args of map are non-fixed, so we set to -1 here. then `operandCount`
can dynamically set according to the number of input args.
I've added some comments here for better readability.
##
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##
@@ -1221,6 +1225,53 @@ private static class MapFromEntriesOperandTypeChecker
}
}
+ /**
+ * Operand type-checking strategy for a MAP function, it allows empty map.
+ */
+ private static class MapFunctionOperandTypeChecker
+ extends SameOperandTypeChecker {
+
+MapFunctionOperandTypeChecker() {
+ super(-1);
+}
+
+@Override public boolean checkOperandTypes(final SqlCallBinding
callBinding,
+final boolean throwOnFailure) {
+ final List argTypes =
+ SqlTypeUtil.deriveType(callBinding, callBinding.operands());
+ // allows empty map
+ if (argTypes.size() == 0) {
+return true;
+ }
+ // the size of map arg types must be even.
+ if (argTypes.size() % 2 > 0) {
+throw
callBinding.newValidationError(RESOURCE.mapRequiresEvenArgCount());
+ }
+ final Pair<@Nullable RelDataType, @Nullable RelDataType> componentType =
+ getComponentTypes(
+ callBinding.getTypeFactory(), argTypes);
+ // check key type & value type
+ if (null == componentType.left || null == componentType.right) {
+if (throwOnFailure) {
+ throw
callBinding.newValidationError(RESOURCE.needSameTypeParameter());
+}
+return false;
+ }
+ return true;
+}
+
+/**
+ * Extract the key type and value type of arg types.
+ */
+private static Pair<@Nullable RelDataType, @Nullable RelDataType>
getComponentTypes(
+RelDataTypeFactory typeFactory,
+List argTypes) {
+ return Pair.of(
+ typeFactory.leastRestrictive(Util.quotientList(argTypes, 2, 0)),
Review Comment:
Util.quotientList(argTypes, 2, 0):
This extracts all elements at even indices from argTypes.
It represents the types of keys in the map as they are placed at even
positions
e.g. 0, 2, 4, etc. details please see Util.quotientList.
I've added some comments here for better readability.
--
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-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459:
URL: https://github.com/apache/calcite/pull/3459#discussion_r1358157015
##
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##
@@ -1221,6 +1225,53 @@ private static class MapFromEntriesOperandTypeChecker
}
}
+ /**
+ * Operand type-checking strategy for a MAP function, it allows empty map.
+ */
+ private static class MapFunctionOperandTypeChecker
+ extends SameOperandTypeChecker {
+
+MapFunctionOperandTypeChecker() {
+ super(-1);
Review Comment:
The args of map are non-fixed, so we set to -1 here. then `operandCount`
can dynamically set according to the number of input args. I've added some
comments for better readability.
--
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-5918] Add MAP function (enabled in Spark library) [calcite]
chucheng92 commented on code in PR #3459:
URL: https://github.com/apache/calcite/pull/3459#discussion_r1358155535
##
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##
@@ -5251,6 +5251,17 @@ public static Map mapFromArrays(List keysArray, List
valuesArray) {
return map;
}
+ /** Support the MAP function. */
+ public static Map mapFunction(Object... args) {
+final Map map = new LinkedHashMap<>();
+for (int i = 0; i < args.length; i++) {
+ Object key = args[i++];
Review Comment:
yes, updated.
--
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-6038] Remove 'ORDER BY ... LIMIT n' when input has at most one row, n >= 1, and there is no 'OFFSET' clause [calcite]
sonarcloud[bot] commented on PR #3467: URL: https://github.com/apache/calcite/pull/3467#issuecomment-1761301995 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3467) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3467=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3467=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_coverage=list) [92.3% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3467=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3467=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
Re: [PR] [CALCITE-6042] Add test cases for ARRAY-related functions by using spark array function (Spark Library) [calcite]
chucheng92 commented on PR #3462:
URL: https://github.com/apache/calcite/pull/3462#issuecomment-1761278157
I have wrote a array-expr-generator, it looks like:
the usage:
```
// array_append(array(1), 2) & array_append(array[1], 2)
f.checkScalarArray(ArrayType.ALL, "[1, 2]", "INTEGER NOT NULL ARRAY NOT
NULL",
"array_append(%A%0, %1)", "1", "2");
// array_append(array(1), null) & array_append(array[1], null)
f.checkScalarArray(ArrayType.ALL, "[1, null]", "INTEGER ARRAY NOT NULL",
"array_append(%A%0, %1)", "1", "null");
// array_append(array(), null)
f.checkScalarArray(ArrayType.FUNCTION, "[null]", "UNKNOWN ARRAY NOT
NULL",
"array_append(%A%0, %1)", "", "null");
// array_append(array(), 1)
f.checkScalarArray(ArrayType.FUNCTION, "[1]", "INTEGER NOT NULL ARRAY
NOT NULL",
"array_append(%A%0, %1)", "", "1");
// array_append(array(array(1,2)), array(3,4)) &
array_append(array[array[1,2]], array[3,4]) -> It's nesting array case.
f.checkScalarArray(ArrayType.ALL, "[[1, 2], [3, 4]]",
"INTEGER NOT NULL ARRAY NOT NULL ARRAY NOT NULL",
```
the implemented method:
```
default void checkScalarArray(
ArrayType syntaxType,
String result,
String resultType,
String exprPattern,
String... exprArgs) {
if (syntaxType == ArrayType.ALL) {
String constructorExpr = generateArrayExpr(true, exprPattern,
exprArgs);
String functionExpr = generateArrayExpr(false, exprPattern, exprArgs);
checkScalar(constructorExpr, result, resultType);
checkScalar(functionExpr, result, resultType);
} else {
boolean useConstructor = syntaxType == ArrayType.CONSTRUCTOR;
String arrayExpr = generateArrayExpr(useConstructor, exprPattern,
exprArgs);
checkScalar(arrayExpr, result, resultType);
}
}
```
E.g., "array_contains(%A%0, %1)". %A[n]%i placeholders are used for array
syntax, where `n` is the optional nesting level and `i` is the index of the
argument to be inserted. %i placeholders without %A will be replaced without
array syntax. exprArgs – Arguments to replace the placeholders in the
expression pattern. They will replace %0, %1, etc.
I feel it will be less readable, but it does remove duplicate code.
I want to know what everyone thinks of this plan? If possible, I will update
all cases to this form. @mihaibudiu what do you think?
--
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-6038] Remove 'ORDER BY ... LIMIT n' when input has at most one row, n >= 1, and there is no 'OFFSET' clause [calcite]
kgyrtkirk commented on code in PR #3467:
URL: https://github.com/apache/calcite/pull/3467#discussion_r1358053936
##
core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRedundantRule.java:
##
@@ -97,13 +110,15 @@ protected SortRemoveRedundantRule(final
SortRemoveRedundantRule.Config config) {
}
private Optional getSortInputSpecificMaxRowCount(Sort sort) {
-// If the sort is pure limit, the specific max row count is limit's fetch.
-if (RelOptUtil.isPureLimit(sort)) {
- final double limit =
- sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
-1D;
- return Optional.of(limit);
+if (RelOptUtil.isLimit(sort)) {
+ final double fetch =
+ sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
0D;
+ if (fetch == 0D) {
+return Optional.empty();
+ }
+ // If sort is 'order by x limit n', the target max row count is 1.
+ return RelOptUtil.isOrder(sort) ? Optional.of(1D) : Optional.of(fetch);
Review Comment:
I feel that answering `1` here based on that this is an `Order` or not is
kinda bending the rules here
I think probably:
* adding a `RelOptUtil.isOrder` outside; as its a little bit different aspect
* or changing the method name to something like
`getMaxRowCountWhenThisSortIsIneffective` could tell the story better
##
core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRedundantRule.java:
##
@@ -97,13 +110,15 @@ protected SortRemoveRedundantRule(final
SortRemoveRedundantRule.Config config) {
}
private Optional getSortInputSpecificMaxRowCount(Sort sort) {
-// If the sort is pure limit, the specific max row count is limit's fetch.
-if (RelOptUtil.isPureLimit(sort)) {
- final double limit =
- sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
-1D;
- return Optional.of(limit);
+if (RelOptUtil.isLimit(sort)) {
+ final double fetch =
Review Comment:
I wonder why double is used around here as only `RexLiteral.intValue` is
used
##
core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRedundantRule.java:
##
@@ -97,13 +110,15 @@ protected SortRemoveRedundantRule(final
SortRemoveRedundantRule.Config config) {
}
private Optional getSortInputSpecificMaxRowCount(Sort sort) {
-// If the sort is pure limit, the specific max row count is limit's fetch.
-if (RelOptUtil.isPureLimit(sort)) {
- final double limit =
- sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
-1D;
- return Optional.of(limit);
+if (RelOptUtil.isLimit(sort)) {
+ final double fetch =
+ sort.fetch instanceof RexLiteral ? RexLiteral.intValue(sort.fetch) :
0D;
+ if (fetch == 0D) {
+return Optional.empty();
+ }
+ // If sort is 'order by x limit n', the target max row count is 1.
+ return RelOptUtil.isOrder(sort) ? Optional.of(1D) : Optional.of(fetch);
} else if (RelOptUtil.isPureOrder(sort)) {
Review Comment:
what if you place a check first in this method:
* limit 0 (does that even happen?)
* check `isOrder` ( rows >=1 implied from prev conditional)
this will kinda do the same...
--
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-5921] SqlOperatorFixture.checkFails and checkAggFails don't check runtime failure [calcite]
mihaibudiu commented on code in PR #3412: URL: https://github.com/apache/calcite/pull/3412#discussion_r1357802519 ## testkit/src/main/java/org/apache/calcite/test/SqlOperatorFixtureImpl.java: ## @@ -163,6 +163,7 @@ void forEachQueryValidateAndThen(StringAndPos expression, SqlValidator validator = factory.createValidator(); SqlNode n = parseAndValidate(validator, sql); assertNotNull(n); + tester.checkFails(factory, sap, expectedError, true); Review Comment: the last parameter to this call should be "runtime" and not "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
Re: [PR] [CALCITE-5921] SqlOperatorFixture.checkFails and checkAggFails don't check runtime failure [calcite]
mihaibudiu commented on code in PR #3412: URL: https://github.com/apache/calcite/pull/3412#discussion_r1357802746 ## testkit/src/main/java/org/apache/calcite/test/SqlOperatorFixtureImpl.java: ## @@ -185,6 +186,7 @@ void forEachQueryValidateAndThen(StringAndPos expression, SqlValidator validator = factory.createValidator(); SqlNode n = parseAndValidate(validator, sql); assertNotNull(n); + tester.checkAggFails(factory, expr, inputValues, expectedError, true); Review Comment: same 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
