You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "liuyongvs (via GitHub)" <gi...@apache.org> on 2023/04/18 16:27:50 UTC
[GitHub] [calcite] liuyongvs opened a new pull request, #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
liuyongvs opened a new pull request, #3161:
URL: https://github.com/apache/calcite/pull/3161
The functions follow Spark's description
ARRAY_DISTINCT: https://spark.apache.org/docs/latest/sql-ref-functions-builtin.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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] MasseGuillaume commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "MasseGuillaume (via GitHub)" <gi...@apache.org>.
MasseGuillaume commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1517181518
I don't have commiters rights. You can find them by looking at the commit logs: https://github.com/apache/calcite/commits/main. (I'm not aware of a exhaustive 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1518921220
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3161)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3161&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3161&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3161&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_duplicated_lines_density&view=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1539276885
Can you rebase `main` and `push -f` to rerun CI?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] MasseGuillaume commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "MasseGuillaume (via GitHub)" <gi...@apache.org>.
MasseGuillaume commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1171711696
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -5167,6 +5167,18 @@ private static void checkIf(SqlOperatorFixture f) {
f.checkNull("array_concat(cast(null as integer array), array[1])");
}
+ /** Tests {@code ARRAY_DISTINCT} function from Spark. */
+ @Test void testArrayDistinctFunc() {
+ SqlOperatorFixture f = fixture()
+ .setFor(SqlLibraryOperators.ARRAY_DISTINCT)
+ .withLibrary(SqlLibrary.SPARK);
+ f.checkScalar("array_distinct(array[1, 2, 2, 1])", "[1, 2]",
+ "INTEGER NOT NULL ARRAY NOT NULL");
+ f.checkScalar("array_distinct(array[null, 1, null])", "[null, 1]",
+ "INTEGER ARRAY NOT NULL");
+ f.checkNull("array_distinct(null)");
+ }
Review Comment:
`array[1, 2]`: is a simpler form, if somethings is wrong with more complex forms (eg `array[1, 2, null]`) it's helpful to debug.
`array[1]`: it's helpful to check for 1 off errors in the return type inference
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1537633752
hi @JiajunBernoulli have squashed the commit.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1541188881
> will it be merged now @JiajunBernoulli
LGTM, merged 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1528717808
@liuyongvs Do you know why the unit test cannot check result?
```
f.checkScalar("array_distinct(array[1, 2, 2, 1])", "[1]",
"INTEGER NOT NULL ARRAY NOT NULL");
```
It also success, but I expect it fail.
[AbstractSqlTester#check](https://github.com/apache/calcite/blob/3326475c766267d521330006cc80730c4e456191/testkit/src/main/java/org/apache/calcite/sql/test/AbstractSqlTester.java#L225) not use resultChecker.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1539958031
hi @JiajunBernoulli ci passed
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1170668719
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3490,6 +3491,12 @@ private static AtomicLong getAtomicLong(String key) {
return atomic;
}
+ /** Support the ARRAY_DISTINCT function. */
+ public static List distinct(List list) {
+ Set result = new LinkedHashSet<>(list);
Review Comment:
this is because the binary compare https://github.com/apache/spark/blob/816ebaca4a81e0a4369b1ffff43a76e23f5e4271/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L4064, but you can refer flink implement i supports.
https://github.com/apache/flink/commit/b9f6a90a9f942e6a69bb31bbe8baf8ab463156d5
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1519911766
hi @libenchao do you have time to review now ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] MasseGuillaume commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "MasseGuillaume (via GitHub)" <gi...@apache.org>.
MasseGuillaume commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1170399078
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3490,6 +3491,12 @@ private static AtomicLong getAtomicLong(String key) {
return atomic;
}
+ /** Support the ARRAY_DISTINCT function. */
+ public static List distinct(List list) {
+ Set result = new LinkedHashSet<>(list);
Review Comment:
I'm not super familiar with SQL distinct semantics, but this might be not enough:
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/util/SQLOpenHashSet.scala#L27
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1537355363
@liuyongvs Would you please squash your commit?
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1539583447
@JiajunBernoulli rebase and force push
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1539599516
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3161)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3161&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3161&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3161&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_duplicated_lines_density&view=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1174256987
##########
site/_docs/reference.md:
##########
@@ -2647,6 +2647,7 @@ BigQuery's type system uses confusingly different names for types and functions:
| m | expr1 <=> expr2 | Whether two values are equal, treating null values as the same, and it's similar to `IS NOT DISTINCT FROM`
| b | ARRAY_CONCAT(array [, array ]*) | Concatenates one or more arrays. If any input argument is `NULL` the function returns `NULL`
| b | ARRAY_LENGTH(array) | Synonym for `CARDINALITY`
+| s | ARRAY_DISTINCT(array) | Returns unique elements of *array*. Keeps ordering of elements.
Review Comment:
Should ARRAY_DISTINCT between ARRAY_CONCAT and ARRAY_LENGTH?
> I like to maintain dictionary order.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1175122427
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -5167,6 +5167,18 @@ private static void checkIf(SqlOperatorFixture f) {
f.checkNull("array_concat(cast(null as integer array), array[1])");
}
+ /** Tests {@code ARRAY_DISTINCT} function from Spark. */
+ @Test void testArrayDistinctFunc() {
+ SqlOperatorFixture f = fixture()
+ .setFor(SqlLibraryOperators.ARRAY_DISTINCT)
+ .withLibrary(SqlLibrary.SPARK);
+ f.checkScalar("array_distinct(array[1, 2, 2, 1])", "[1, 2]",
+ "INTEGER NOT NULL ARRAY NOT NULL");
+ f.checkScalar("array_distinct(array[null, 1, null])", "[null, 1]",
+ "INTEGER ARRAY NOT NULL");
+ f.checkNull("array_distinct(null)");
+ }
Review Comment:
don't need. array should be element with same type, and it doesn't support implicit cast. so i guess it will throw exception.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1533962959
> @liuyongvs Do you know why the unit test cannot check result?
>
> ```
> f.checkScalar("array_distinct(array[1, 2, 2, 1])", "[1]",
> "INTEGER NOT NULL ARRAY NOT NULL");
> ```
>
> It also success, but I expect it fail.
>
> [AbstractSqlTester#check](https://github.com/apache/calcite/blob/3326475c766267d521330006cc80730c4e456191/testkit/src/main/java/org/apache/calcite/sql/test/AbstractSqlTester.java#L225) not use resultChecker.
you can use this, the default unit test just check type, and the itcase will check the result, this is for less time to run the tests
final SqlOperatorFixture f = Fixtures.forOperators(true)
.setFor(SqlLibraryOperators.XXX)
.withLibrary(SqlLibrary.SPARK);
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli merged pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli merged PR #3161:
URL: https://github.com/apache/calcite/pull/3161
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1536205871
hi @libenchao @JiajunBernoulli you have a look 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1517144286
hi @MasseGuillaume will you merge 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1517186666
@liuyongvs Thanks for your contribution, and @MasseGuillaume thanks for your review.
I assigned this to myself, if there is no other Committer take this, I'll review this in a few days.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1170666492
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -5167,6 +5167,18 @@ private static void checkIf(SqlOperatorFixture f) {
f.checkNull("array_concat(cast(null as integer array), array[1])");
}
+ /** Tests {@code ARRAY_DISTINCT} function from Spark. */
+ @Test void testArrayDistinctFunc() {
+ SqlOperatorFixture f = fixture()
+ .setFor(SqlLibraryOperators.ARRAY_DISTINCT)
+ .withLibrary(SqlLibrary.SPARK);
+ f.checkScalar("array_distinct(array[1, 2, 2, 1])", "[1, 2]",
+ "INTEGER NOT NULL ARRAY NOT NULL");
+ f.checkScalar("array_distinct(array[null, 1, null])", "[null, 1]",
+ "INTEGER ARRAY NOT NULL");
+ f.checkNull("array_distinct(null)");
+ }
Review Comment:
why need this, it is the same. unit tests need to be carefully selected
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1541178332
will it be merged now @JiajunBernoulli
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1174258550
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -5167,6 +5167,18 @@ private static void checkIf(SqlOperatorFixture f) {
f.checkNull("array_concat(cast(null as integer array), array[1])");
}
+ /** Tests {@code ARRAY_DISTINCT} function from Spark. */
Review Comment:
```
/**
* Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-5657"> [CALCITE-5657]
* Add ARRAY_DISTINCT function (enabled in Spark library)
* </a>.
*/
```
Maybe better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1175122427
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -5167,6 +5167,18 @@ private static void checkIf(SqlOperatorFixture f) {
f.checkNull("array_concat(cast(null as integer array), array[1])");
}
+ /** Tests {@code ARRAY_DISTINCT} function from Spark. */
+ @Test void testArrayDistinctFunc() {
+ SqlOperatorFixture f = fixture()
+ .setFor(SqlLibraryOperators.ARRAY_DISTINCT)
+ .withLibrary(SqlLibrary.SPARK);
+ f.checkScalar("array_distinct(array[1, 2, 2, 1])", "[1, 2]",
+ "INTEGER NOT NULL ARRAY NOT NULL");
+ f.checkScalar("array_distinct(array[null, 1, null])", "[null, 1]",
+ "INTEGER ARRAY NOT NULL");
+ f.checkNull("array_distinct(null)");
+ }
Review Comment:
don't need. array should be element with same type, and it doesn't support implicit cast. so i guess it will throw exception. @JiajunBernoulli
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] MasseGuillaume commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "MasseGuillaume (via GitHub)" <gi...@apache.org>.
MasseGuillaume commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1170372796
##########
site/_docs/reference.md:
##########
@@ -2647,6 +2647,7 @@ BigQuery's type system uses confusingly different names for types and functions:
| m | expr1 <=> expr2 | Whether two values are equal, treating null values as the same, and it's similar to `IS NOT DISTINCT FROM`
| b | ARRAY_CONCAT(array [, array ]*) | Concatenates one or more arrays. If any input argument is `NULL` the function returns `NULL`
| b | ARRAY_LENGTH(array) | Synonym for `CARDINALITY`
+| b | ARRAY_DISTINCT(array) | Returns unique elements of *array*. Keeps ordering of elements.
Review Comment:
s
https://ci-builds.apache.org/blue/organizations/jenkins/Calcite%2FCalcite-sonar/detail/PR-3161/1/pipeline#step-19-log-290
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] MasseGuillaume commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "MasseGuillaume (via GitHub)" <gi...@apache.org>.
MasseGuillaume commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1170380220
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -5167,6 +5167,18 @@ private static void checkIf(SqlOperatorFixture f) {
f.checkNull("array_concat(cast(null as integer array), array[1])");
}
+ /** Tests {@code ARRAY_DISTINCT} function from Spark. */
+ @Test void testArrayDistinctFunc() {
+ SqlOperatorFixture f = fixture()
+ .setFor(SqlLibraryOperators.ARRAY_DISTINCT)
+ .withLibrary(SqlLibrary.SPARK);
+ f.checkScalar("array_distinct(array[1, 2, 2, 1])", "[1, 2]",
+ "INTEGER NOT NULL ARRAY NOT NULL");
+ f.checkScalar("array_distinct(array[null, 1, null])", "[null, 1]",
+ "INTEGER ARRAY NOT NULL");
+ f.checkNull("array_distinct(null)");
+ }
Review Comment:
once we have https://github.com/apache/calcite/pull/3141 merged, you will be able to check type for empty:
```scala
spark.sql("SELECT array_distinct(array())")
res3: org.apache.spark.sql.DataFrame = [array_distinct(array()): array<void>]
```
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -5167,6 +5167,18 @@ private static void checkIf(SqlOperatorFixture f) {
f.checkNull("array_concat(cast(null as integer array), array[1])");
}
+ /** Tests {@code ARRAY_DISTINCT} function from Spark. */
+ @Test void testArrayDistinctFunc() {
+ SqlOperatorFixture f = fixture()
+ .setFor(SqlLibraryOperators.ARRAY_DISTINCT)
+ .withLibrary(SqlLibrary.SPARK);
+ f.checkScalar("array_distinct(array[1, 2, 2, 1])", "[1, 2]",
+ "INTEGER NOT NULL ARRAY NOT NULL");
+ f.checkScalar("array_distinct(array[null, 1, null])", "[null, 1]",
+ "INTEGER ARRAY NOT NULL");
+ f.checkNull("array_distinct(null)");
+ }
Review Comment:
missing happy path:
```suggestion
f.checkScalar("array_distinct(array[1, 2])", "[1, 2]",
"INTEGER NOT NULL ARRAY NOT NULL");
f.checkScalar("array_distinct(array[1])", "[1]",
"INTEGER NOT NULL ARRAY NOT NULL");
}
```
##########
site/_docs/reference.md:
##########
@@ -2647,6 +2647,7 @@ BigQuery's type system uses confusingly different names for types and functions:
| m | expr1 <=> expr2 | Whether two values are equal, treating null values as the same, and it's similar to `IS NOT DISTINCT FROM`
| b | ARRAY_CONCAT(array [, array ]*) | Concatenates one or more arrays. If any input argument is `NULL` the function returns `NULL`
| b | ARRAY_LENGTH(array) | Synonym for `CARDINALITY`
+| b | ARRAY_DISTINCT(array) | Returns unique elements of *array*. Keeps ordering of elements.
Review Comment:
s
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1513951457
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3161)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3161&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3161&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3161&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3161&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3161&metric=new_duplicated_lines_density&view=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1521594850
@liuyongvs Since @JiajunBernoulli has started to review the PR, and he has the commit authority, I would like to defer this to him unless he is not available.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1174492693
##########
site/_docs/reference.md:
##########
@@ -2647,6 +2647,7 @@ BigQuery's type system uses confusingly different names for types and functions:
| m | expr1 <=> expr2 | Whether two values are equal, treating null values as the same, and it's similar to `IS NOT DISTINCT FROM`
| b | ARRAY_CONCAT(array [, array ]*) | Concatenates one or more arrays. If any input argument is `NULL` the function returns `NULL`
| b | ARRAY_LENGTH(array) | Synonym for `CARDINALITY`
+| s | ARRAY_DISTINCT(array) | Returns unique elements of *array*. Keeps ordering of elements.
Review Comment:
fixed @JiajunBernoulli
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1174492785
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -5167,6 +5167,18 @@ private static void checkIf(SqlOperatorFixture f) {
f.checkNull("array_concat(cast(null as integer array), array[1])");
}
+ /** Tests {@code ARRAY_DISTINCT} function from Spark. */
Review Comment:
may not. you can refer other functions like array_remove in the calcite code
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a diff in pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on code in PR #3161:
URL: https://github.com/apache/calcite/pull/3161#discussion_r1174812427
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -5167,6 +5167,18 @@ private static void checkIf(SqlOperatorFixture f) {
f.checkNull("array_concat(cast(null as integer array), array[1])");
}
+ /** Tests {@code ARRAY_DISTINCT} function from Spark. */
+ @Test void testArrayDistinctFunc() {
+ SqlOperatorFixture f = fixture()
+ .setFor(SqlLibraryOperators.ARRAY_DISTINCT)
+ .withLibrary(SqlLibrary.SPARK);
+ f.checkScalar("array_distinct(array[1, 2, 2, 1])", "[1, 2]",
+ "INTEGER NOT NULL ARRAY NOT NULL");
+ f.checkScalar("array_distinct(array[null, 1, null])", "[null, 1]",
+ "INTEGER ARRAY NOT NULL");
+ f.checkNull("array_distinct(null)");
+ }
Review Comment:
How about `array_distinct(array[0, 0.0])`?
Should we need unit tests for array<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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on pull request #3161: [CALCITE-5657] Add ARRAY_DISTINCT function (enabled in Spark library).
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3161:
URL: https://github.com/apache/calcite/pull/3161#issuecomment-1537828435
hi @JiajunBernoulli
Task :sonar
[java.net.SocketTimeoutException: Connect timed out, java.net.SocketTimeoutException: Connect timed out]
what should i do? and what is your dingding or wechat ?
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org