You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/03/27 08:38:57 UTC
[GitHub] [calcite] wangxlong opened a new pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
wangxlong opened a new pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878
According to the discussion [CALCITE-3732](https://issues.apache.org/jira/browse/CALCITE-3732) , We should make bitwise operators work on all integer types, BINARY and VARBINARY. So Bit_And, Bit_OR and Bit_XOR agg operator should also support BINARY and VARBINARY.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] wangxlong commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
wangxlong commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r410117600
##########
File path: core/src/main/java/org/apache/calcite/runtime/BitwiseFunctions.java
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.runtime;
+
+import org.apache.calcite.avatica.util.ByteString;
+
+import java.util.stream.IntStream;
+
+import static org.apache.calcite.util.Static.RESOURCE;
+
+/**
+ * A collection of functions used in Bitwise processing.
+ */
+public class BitwiseFunctions {
+
+ private BitwiseFunctions(){}
+
+ // &
+ /** Helper function for implementing <code>BIT_AND</code> applied to integer values */
+ public static long bitAnd(long b0, long b1) {
+ return b0 & b1;
+ }
+
+ /** Helper function for implementing <code>BIT_AND</code> applied to binary values */
+ public static ByteString bitAnd(ByteString b0, ByteString b1) {
+
+ return binaryOperator(b0, b1, BitwiseOperators.BITAND);
+ }
+
+ // |
+ /** Helper function for implementing <code>BIT_OR</code> applied to integer values */
+ public static long bitOr(long b0, long b1) {
+ return b0 | b1;
+ }
+
+ /** Helper function for implementing <code>BIT_OR</code> applied to binary values */
+ public static ByteString bitOr(ByteString b0, ByteString b1) {
+ return binaryOperator(b0, b1, BitwiseOperators.BITOR);
+ }
+
+ // ^
+ /** Helper function for implementing <code>BIT_XOR</code> applied to integer values */
+ public static long bitXor(long b0, long b1) {
+ return b0 ^ b1;
+ }
+
+ /** Helper function for implementing <code>BIT_XOR</code> applied to binary values */
+ public static ByteString bitXor(ByteString b0, ByteString b1) {
+
+ return binaryOperator(b0, b1, BitwiseOperators.BITXOR);
+ }
+
+ /**
+ * Util for bitwise function applied to two byteString values.
+ * @param b0 The first byteString value operand of bitwise function.
+ * @param b1 The second byteString value operand of bitwise function.
+ * @param operator BitWise Operator.
+ * @return
+ */
+ private static ByteString binaryOperator(
+ ByteString b0, ByteString b1, BitwiseOperators operator) {
Review comment:
Of course. Thank you for your suggestion. Updates.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r409653172
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -9096,6 +9114,12 @@ protected void checkAggType(SqlTester tester, String expr, String type) {
false);
final String[] values = {"1", "2", "1"};
tester.checkAgg("bit_xor(x)", values, 2, 0);
+ final String[] binaryValues = {
+ "CAST(x'01' AS BINARY)",
+ "cast(x'02' as BINARY)",
+ "cast(x'01' AS BINARY)",
+ "cast(null AS BINARY)"};
+ tester.checkAgg("bit_xor(x)", binaryValues, "02", 0);
Review comment:
Correct.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] wangxlong commented on issue #1878: [CALCITE-3782]Bitwise
agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
wangxlong commented on issue #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#issuecomment-611333982
@chunweilei Thanks for your advice. I have 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r399613737
##########
File path: core/src/main/java/org/apache/calcite/runtime/CalciteResource.java
##########
@@ -918,4 +918,7 @@
@BaseMessage("Invalid input for EXTRACTVALUE: xml: ''{0}'', xpath expression: ''{1}''")
ExInst<CalciteException> invalidInputForExtractValue(String xml, String xpath);
+
+ @BaseMessage("Invalid length for bitwise operator of input binary value")
+ ExInst<CalciteException> invalidInputForBitwiseBinaryValue();
Review comment:
Actually the error throws because the two params have different length.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] wangxlong commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
wangxlong commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r407122867
##########
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##########
@@ -1074,23 +1075,92 @@ private static RuntimeException notComparable(String op, Object b0,
}
// &
- /** Helper function for implementing <code>BIT_AND</code> */
+ /** Helper function for implementing <code>BIT_AND</code> applied to integer values */
public static long bitAnd(long b0, long b1) {
return b0 & b1;
}
+ /** Helper function for implementing <code>BIT_AND</code> applied to binary values */
+ public static ByteString bitAnd(ByteString b0, ByteString b1) {
+
+ if (b0.length() == 0) {
+ return b1;
+ }
+ if (b1.length() == 0) {
+ return b0;
+ }
+
+ if (b0.length() != b1.length()) {
+ throw RESOURCE.differentLengthForBitwiseOperands(b0.length(), b1.length()).ex();
+ }
+ byte[] bytes0 = b0.getBytes();
+ byte[] bytes1 = b1.getBytes();
+ byte[] result = new byte[b0.length()];
+
+ IntStream.range(0, bytes0.length).forEach(i -> {
+ result[i] = (byte) (bytes0[i] & bytes1[i]);
+ });
+ return new ByteString(result);
+ }
+
// |
- /** Helper function for implementing <code>BIT_OR</code> */
+ /** Helper function for implementing <code>BIT_OR</code> applied to integer values */
public static long bitOr(long b0, long b1) {
return b0 | b1;
}
+ /** Helper function for implementing <code>BIT_OR</code> applied to binary values */
+ public static ByteString bitOr(ByteString b0, ByteString b1) {
+
+ if (b0.length() == 0) {
+ return b1;
+ }
+ if (b1.length() == 0) {
+ return b0;
+ }
+
+ if (b0.length() != b1.length()) {
+ throw RESOURCE.differentLengthForBitwiseOperands(b0.length(), b1.length()).ex();
+ }
+ byte[] bytes0 = b0.getBytes();
+ byte[] bytes1 = b1.getBytes();
+ byte[] result = new byte[b0.length()];
+
+ IntStream.range(0, bytes0.length).forEach(i -> {
Review comment:
Thanks @hsyuan @ritesh-kapoor. Yes, moved it to utility function looks more better. Thank you help me to reivew again in your free time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r409949850
##########
File path: core/src/main/java/org/apache/calcite/runtime/BitwiseFunctions.java
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.runtime;
+
+import org.apache.calcite.avatica.util.ByteString;
+
+import java.util.stream.IntStream;
+
+import static org.apache.calcite.util.Static.RESOURCE;
+
+/**
+ * A collection of functions used in Bitwise processing.
+ */
+public class BitwiseFunctions {
+
+ private BitwiseFunctions(){}
+
+ // &
+ /** Helper function for implementing <code>BIT_AND</code> applied to integer values */
+ public static long bitAnd(long b0, long b1) {
+ return b0 & b1;
+ }
+
+ /** Helper function for implementing <code>BIT_AND</code> applied to binary values */
+ public static ByteString bitAnd(ByteString b0, ByteString b1) {
+
+ return binaryOperator(b0, b1, BitwiseOperators.BITAND);
+ }
+
+ // |
+ /** Helper function for implementing <code>BIT_OR</code> applied to integer values */
+ public static long bitOr(long b0, long b1) {
+ return b0 | b1;
+ }
+
+ /** Helper function for implementing <code>BIT_OR</code> applied to binary values */
+ public static ByteString bitOr(ByteString b0, ByteString b1) {
+ return binaryOperator(b0, b1, BitwiseOperators.BITOR);
+ }
+
+ // ^
+ /** Helper function for implementing <code>BIT_XOR</code> applied to integer values */
+ public static long bitXor(long b0, long b1) {
+ return b0 ^ b1;
+ }
+
+ /** Helper function for implementing <code>BIT_XOR</code> applied to binary values */
+ public static ByteString bitXor(ByteString b0, ByteString b1) {
+
+ return binaryOperator(b0, b1, BitwiseOperators.BITXOR);
+ }
+
+ /**
+ * Util for bitwise function applied to two byteString values.
+ * @param b0 The first byteString value operand of bitwise function.
+ * @param b1 The second byteString value operand of bitwise function.
+ * @param operator BitWise Operator.
+ * @return
+ */
+ private static ByteString binaryOperator(
+ ByteString b0, ByteString b1, BitwiseOperators operator) {
Review comment:
Can we use `Supplier` like https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/plan/RelTraitSet.java#L232 instead of BitwiseOperators enum?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r406204922
##########
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##########
@@ -1074,23 +1075,92 @@ private static RuntimeException notComparable(String op, Object b0,
}
// &
- /** Helper function for implementing <code>BIT_AND</code> */
+ /** Helper function for implementing <code>BIT_AND</code> applied to integer values */
public static long bitAnd(long b0, long b1) {
return b0 & b1;
}
+ /** Helper function for implementing <code>BIT_AND</code> applied to binary values */
+ public static ByteString bitAnd(ByteString b0, ByteString b1) {
+
+ if (b0.length() == 0) {
+ return b1;
+ }
+ if (b1.length() == 0) {
+ return b0;
+ }
+
+ if (b0.length() != b1.length()) {
+ throw RESOURCE.differentLengthForBitwiseOperands(b0.length(), b1.length()).ex();
+ }
+ byte[] bytes0 = b0.getBytes();
+ byte[] bytes1 = b1.getBytes();
+ byte[] result = new byte[b0.length()];
+
+ IntStream.range(0, bytes0.length).forEach(i -> {
+ result[i] = (byte) (bytes0[i] & bytes1[i]);
+ });
+ return new ByteString(result);
+ }
+
// |
- /** Helper function for implementing <code>BIT_OR</code> */
+ /** Helper function for implementing <code>BIT_OR</code> applied to integer values */
public static long bitOr(long b0, long b1) {
return b0 | b1;
}
+ /** Helper function for implementing <code>BIT_OR</code> applied to binary values */
+ public static ByteString bitOr(ByteString b0, ByteString b1) {
+
+ if (b0.length() == 0) {
+ return b1;
+ }
+ if (b1.length() == 0) {
+ return b0;
+ }
+
+ if (b0.length() != b1.length()) {
+ throw RESOURCE.differentLengthForBitwiseOperands(b0.length(), b1.length()).ex();
+ }
+ byte[] bytes0 = b0.getBytes();
+ byte[] bytes1 = b1.getBytes();
+ byte[] result = new byte[b0.length()];
+
+ IntStream.range(0, bytes0.length).forEach(i -> {
Review comment:
I think we should do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] wangxlong commented on issue #1878: [CALCITE-3782]Bitwise
agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
wangxlong commented on issue #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#issuecomment-605405465
@danny0405 Thanks for your advice, I have 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] wangxlong commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
wangxlong commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r407122867
##########
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##########
@@ -1074,23 +1075,92 @@ private static RuntimeException notComparable(String op, Object b0,
}
// &
- /** Helper function for implementing <code>BIT_AND</code> */
+ /** Helper function for implementing <code>BIT_AND</code> applied to integer values */
public static long bitAnd(long b0, long b1) {
return b0 & b1;
}
+ /** Helper function for implementing <code>BIT_AND</code> applied to binary values */
+ public static ByteString bitAnd(ByteString b0, ByteString b1) {
+
+ if (b0.length() == 0) {
+ return b1;
+ }
+ if (b1.length() == 0) {
+ return b0;
+ }
+
+ if (b0.length() != b1.length()) {
+ throw RESOURCE.differentLengthForBitwiseOperands(b0.length(), b1.length()).ex();
+ }
+ byte[] bytes0 = b0.getBytes();
+ byte[] bytes1 = b1.getBytes();
+ byte[] result = new byte[b0.length()];
+
+ IntStream.range(0, bytes0.length).forEach(i -> {
+ result[i] = (byte) (bytes0[i] & bytes1[i]);
+ });
+ return new ByteString(result);
+ }
+
// |
- /** Helper function for implementing <code>BIT_OR</code> */
+ /** Helper function for implementing <code>BIT_OR</code> applied to integer values */
public static long bitOr(long b0, long b1) {
return b0 | b1;
}
+ /** Helper function for implementing <code>BIT_OR</code> applied to binary values */
+ public static ByteString bitOr(ByteString b0, ByteString b1) {
+
+ if (b0.length() == 0) {
+ return b1;
+ }
+ if (b1.length() == 0) {
+ return b0;
+ }
+
+ if (b0.length() != b1.length()) {
+ throw RESOURCE.differentLengthForBitwiseOperands(b0.length(), b1.length()).ex();
+ }
+ byte[] bytes0 = b0.getBytes();
+ byte[] bytes1 = b1.getBytes();
+ byte[] result = new byte[b0.length()];
+
+ IntStream.range(0, bytes0.length).forEach(i -> {
Review comment:
Thanks @hsyuan @ritesh-kapoor. Yes, moved it to utility function looks more better. Thank you help me to have a look in your free time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r408243693
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -9096,6 +9114,12 @@ protected void checkAggType(SqlTester tester, String expr, String type) {
false);
final String[] values = {"1", "2", "1"};
tester.checkAgg("bit_xor(x)", values, 2, 0);
+ final String[] binaryValues = {
+ "CAST(x'01' AS BINARY)",
+ "cast(x'02' as BINARY)",
+ "cast(x'01' AS BINARY)",
+ "cast(null AS BINARY)"};
+ tester.checkAgg("bit_xor(x)", binaryValues, "02", 0);
Review comment:
Can you add a case with duplicate element and distinct for BIT_XOR? I am afraid it won't work.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r399615184
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -9074,6 +9083,11 @@ protected void checkAggType(SqlTester tester, String expr, String type) {
false);
final String[] values = {"1", "2", "2"};
tester.checkAgg("bit_or(x)", values, 3, 0);
+ final String[] binaryValues = {
+ "CAST(x'01' AS BINARY)",
+ "cast(x'02' as BINARY)",
+ "cast(x'02' AS BINARY)"};
Review comment:
Add a test for `null` value.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r399613856
##########
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##########
@@ -1074,23 +1075,83 @@ private static RuntimeException notComparable(String op, Object b0,
}
// &
- /** Helper function for implementing <code>BIT_AND</code> */
+ /** Helper function for implementing <code>BIT_AND</code> applied to integer values */
public static long bitAnd(long b0, long b1) {
return b0 & b1;
}
+ /** Helper function for implementing <code>BIT_AND</code> applied to binary values */
+ public static ByteString bitAnd(ByteString b0, ByteString b1) {
+
+ if (b0.length() == 0 || b1.length() == 0) {
+ return b1.length() == 0 ? b0 : b1;
+ }
+
Review comment:
The code below seems more straight-forward.
```java
if (b0.length() == 0) {
return b1;
}
if (b1.length() == 0) {
return b0;
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r399615214
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -9074,6 +9083,11 @@ protected void checkAggType(SqlTester tester, String expr, String type) {
false);
final String[] values = {"1", "2", "2"};
tester.checkAgg("bit_or(x)", values, 3, 0);
+ final String[] binaryValues = {
+ "CAST(x'01' AS BINARY)",
+ "cast(x'02' as BINARY)",
+ "cast(x'02' AS BINARY)"};
Review comment:
We may also need to update the `reference.md` for the function signatures change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r407236690
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -9096,6 +9114,12 @@ protected void checkAggType(SqlTester tester, String expr, String type) {
false);
final String[] values = {"1", "2", "1"};
tester.checkAgg("bit_xor(x)", values, 2, 0);
+ final String[] binaryValues = {
+ "CAST(x'01' AS BINARY)",
+ "cast(x'02' as BINARY)",
+ "cast(x'01' AS BINARY)",
+ "cast(null AS BINARY)"};
+ tester.checkAgg("bit_xor(x)", binaryValues, "02", 0);
Review comment:
Can you add the case?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] wangxlong commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
wangxlong commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r405959068
##########
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##########
@@ -1074,23 +1075,92 @@ private static RuntimeException notComparable(String op, Object b0,
}
// &
- /** Helper function for implementing <code>BIT_AND</code> */
+ /** Helper function for implementing <code>BIT_AND</code> applied to integer values */
public static long bitAnd(long b0, long b1) {
return b0 & b1;
}
+ /** Helper function for implementing <code>BIT_AND</code> applied to binary values */
+ public static ByteString bitAnd(ByteString b0, ByteString b1) {
+
+ if (b0.length() == 0) {
+ return b1;
+ }
+ if (b1.length() == 0) {
+ return b0;
+ }
+
+ if (b0.length() != b1.length()) {
+ throw RESOURCE.differentLengthForBitwiseOperands(b0.length(), b1.length()).ex();
+ }
+ byte[] bytes0 = b0.getBytes();
+ byte[] bytes1 = b1.getBytes();
+ byte[] result = new byte[b0.length()];
+
+ IntStream.range(0, bytes0.length).forEach(i -> {
+ result[i] = (byte) (bytes0[i] & bytes1[i]);
+ });
+ return new ByteString(result);
+ }
+
// |
- /** Helper function for implementing <code>BIT_OR</code> */
+ /** Helper function for implementing <code>BIT_OR</code> applied to integer values */
public static long bitOr(long b0, long b1) {
return b0 | b1;
}
+ /** Helper function for implementing <code>BIT_OR</code> applied to binary values */
+ public static ByteString bitOr(ByteString b0, ByteString b1) {
+
+ if (b0.length() == 0) {
+ return b1;
+ }
+ if (b1.length() == 0) {
+ return b0;
+ }
+
+ if (b0.length() != b1.length()) {
+ throw RESOURCE.differentLengthForBitwiseOperands(b0.length(), b1.length()).ex();
+ }
+ byte[] bytes0 = b0.getBytes();
+ byte[] bytes1 = b1.getBytes();
+ byte[] result = new byte[b0.length()];
+
+ IntStream.range(0, bytes0.length).forEach(i -> {
Review comment:
Sorry for reply later. The variable definition of the function is the same, but the main processing mode is different between BIT_AND, BIT_OR, BIT_XOR which are &, | and ^. So We don't need to move it to utility function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r409905987
##########
File path: core/src/main/java/org/apache/calcite/sql/fun/SqlBitOpAggFunction.java
##########
@@ -62,6 +62,17 @@ public SqlBitOpAggFunction(SqlKind kind) {
}
@Override public Optionality getDistinctOptionality() {
- return Optionality.IGNORED;
+ final Optionality optionality;
+
+ switch (kind) {
+ case BIT_AND:
+ case BIT_OR:
+ optionality = Optionality.IGNORED;
+ break;
+ default:
Review comment:
Can you add BIT_XOR before default?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r407236633
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -9096,6 +9114,12 @@ protected void checkAggType(SqlTester tester, String expr, String type) {
false);
final String[] values = {"1", "2", "1"};
tester.checkAgg("bit_xor(x)", values, 2, 0);
+ final String[] binaryValues = {
+ "CAST(x'01' AS BINARY)",
+ "cast(x'02' as BINARY)",
+ "cast(x'01' AS BINARY)",
+ "cast(null AS BINARY)"};
+ tester.checkAgg("bit_xor(x)", binaryValues, "02", 0);
Review comment:
What if it has only 1 element?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] wangxlong commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
wangxlong commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r409906652
##########
File path: core/src/main/java/org/apache/calcite/sql/fun/SqlBitOpAggFunction.java
##########
@@ -62,6 +62,17 @@ public SqlBitOpAggFunction(SqlKind kind) {
}
@Override public Optionality getDistinctOptionality() {
- return Optionality.IGNORED;
+ final Optionality optionality;
+
+ switch (kind) {
+ case BIT_AND:
+ case BIT_OR:
+ optionality = Optionality.IGNORED;
+ break;
+ default:
Review comment:
Yes, It is ok.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] ritesh-kapoor commented on a change in pull request
#1878: [CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
ritesh-kapoor commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r400761658
##########
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##########
@@ -1074,23 +1075,92 @@ private static RuntimeException notComparable(String op, Object b0,
}
// &
- /** Helper function for implementing <code>BIT_AND</code> */
+ /** Helper function for implementing <code>BIT_AND</code> applied to integer values */
public static long bitAnd(long b0, long b1) {
return b0 & b1;
}
+ /** Helper function for implementing <code>BIT_AND</code> applied to binary values */
+ public static ByteString bitAnd(ByteString b0, ByteString b1) {
+
+ if (b0.length() == 0) {
+ return b1;
+ }
+ if (b1.length() == 0) {
+ return b0;
+ }
+
+ if (b0.length() != b1.length()) {
+ throw RESOURCE.differentLengthForBitwiseOperands(b0.length(), b1.length()).ex();
+ }
+ byte[] bytes0 = b0.getBytes();
+ byte[] bytes1 = b1.getBytes();
+ byte[] result = new byte[b0.length()];
+
+ IntStream.range(0, bytes0.length).forEach(i -> {
+ result[i] = (byte) (bytes0[i] & bytes1[i]);
+ });
+ return new ByteString(result);
+ }
+
// |
- /** Helper function for implementing <code>BIT_OR</code> */
+ /** Helper function for implementing <code>BIT_OR</code> applied to integer values */
public static long bitOr(long b0, long b1) {
return b0 | b1;
}
+ /** Helper function for implementing <code>BIT_OR</code> applied to binary values */
+ public static ByteString bitOr(ByteString b0, ByteString b1) {
+
+ if (b0.length() == 0) {
+ return b1;
+ }
+ if (b1.length() == 0) {
+ return b0;
+ }
+
+ if (b0.length() != b1.length()) {
+ throw RESOURCE.differentLengthForBitwiseOperands(b0.length(), b1.length()).ex();
+ }
+ byte[] bytes0 = b0.getBytes();
+ byte[] bytes1 = b1.getBytes();
+ byte[] result = new byte[b0.length()];
+
+ IntStream.range(0, bytes0.length).forEach(i -> {
Review comment:
Should we move it to utility function. Seems like repetitive.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] wangxlong commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
wangxlong commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r409588576
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -9096,6 +9114,12 @@ protected void checkAggType(SqlTester tester, String expr, String type) {
false);
final String[] values = {"1", "2", "1"};
tester.checkAgg("bit_xor(x)", values, 2, 0);
+ final String[] binaryValues = {
+ "CAST(x'01' AS BINARY)",
+ "cast(x'02' as BINARY)",
+ "cast(x'01' AS BINARY)",
+ "cast(null AS BINARY)"};
+ tester.checkAgg("bit_xor(x)", binaryValues, "02", 0);
Review comment:
You are right. Distinct for BIT_XOR isn't work. For SqlBitOpAggFunction#getDistinctOptionality is Optionality.IGNORED. So I think, for BIT_AND and BIT_OR, We can set Optionality.IGNORED. And for BIT_XOR, We can set Optionality.OPTIONAL. What do you think of 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] wangxlong commented on a change in pull request #1878:
[CALCITE-3782]Bitwise agg operator Bit_And,
Bit_OR and Bit_XOR support binary and varbinary type
Posted by GitBox <gi...@apache.org>.
wangxlong commented on a change in pull request #1878: [CALCITE-3782]Bitwise agg operator Bit_And, Bit_OR and Bit_XOR support binary and varbinary type
URL: https://github.com/apache/calcite/pull/1878#discussion_r407423056
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -9096,6 +9114,12 @@ protected void checkAggType(SqlTester tester, String expr, String type) {
false);
final String[] values = {"1", "2", "1"};
tester.checkAgg("bit_xor(x)", values, 2, 0);
+ final String[] binaryValues = {
+ "CAST(x'01' AS BINARY)",
+ "cast(x'02' as BINARY)",
+ "cast(x'01' AS BINARY)",
+ "cast(null AS BINARY)"};
+ tester.checkAgg("bit_xor(x)", binaryValues, "02", 0);
Review comment:
Yes, I have added. When it has only 1 element, the result is itself.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services