[CALCITE-5848] Add BIT_GET and GETBIT functions (enabled in Spark library)#3319
[CALCITE-5848] Add BIT_GET and GETBIT functions (enabled in Spark library)#3319zoudan merged 1 commit intoapache:mainfrom
Conversation
|
@tanclary would you like to help review if you have time? Thanks very much. |
| f1.checkNull("BIT_LENGTH(cast(null as binary))"); | ||
| } | ||
|
|
||
| @Test void testBitGetFunc() { |
There was a problem hiding this comment.
Maybe we could reuse the test code for BIT_GET and GETBIT, as the only difference between them is the function name.
There was a problem hiding this comment.
Would you mean to reuse the test code by parameterized test?
By the way, currently testGetBitFunc only contains basic test, and testBitGetFunc contains the full test cases.
There was a problem hiding this comment.
Yes, in that way we could test all cases for both of them.
site/_docs/reference.md
Outdated
| | * | ATANH(numeric) | Returns the inverse hyperbolic tangent of *numeric* | ||
| | s | BIT_LENGTH(binary) | Returns the bit length of *binary* | ||
| | s | BIT_LENGTH(string) | Returns the bit length of *string* | ||
| | s | BIT_GET(value, position) | Returns the bit (0 or 1) value at the specified *position* of *value*. The positions are numbered from right to left, starting at zero. The *position* argument cannot be negative |
There was a problem hiding this comment.
What types are supported for value?
There was a problem hiding this comment.
The supported type is NUMERIC type.
There was a problem hiding this comment.
I will update the doc to address this.
| } | ||
|
|
||
| /** SQL BIT_GET(value, position) function. */ | ||
| public static int bitGet(long value, int position) { |
There was a problem hiding this comment.
Shall we take other number types like Decimal into consideration?
There was a problem hiding this comment.
According to Spark's PR to implement BIT_GET function, currently we don't support decimal type. For consistency, I think we should not take other number types like Decimal into consideration.
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
Outdated
Show resolved
Hide resolved
|
@zoudan Thanks for the review. |
|
LGTM, would you mind rebase and squash your commits? |
zoudan
added
the
LGTM-will-merge-soon
herunkang2018
force-pushed
the
CALCITE-5848
branch
from
8726afb to
5ac75ef
Compare
|
@zoudan The commits is squashed. Thanks for the valuable comments in the review. |
|
Sorry, one minor comment use |
herunkang2018
changed the title
herunkang2018
force-pushed
the
CALCITE-5848
branch
from
5ac75ef to
04190d3
Compare
|
@zoudan Good catch. This is fixed. |
|
Kudos, SonarCloud Quality Gate passed! |
2 participants
No description provided.