Skip to content

[CALCITE-3779] Implement bitand scalar function#1795

Closed
wangxlong wants to merge 5 commits intoapache:mainfrom
wangxlong:CALCITE-3779
Closed

[CALCITE-3779] Implement bitand scalar function#1795
wangxlong wants to merge 5 commits intoapache:mainfrom
wangxlong:CALCITE-3779

Conversation

@wangxlong
Copy link
Contributor

Implement bitand scalar function which support tinyint, smallint, int, bigint, binary and varbinary type.
Related issue: CALCITE-3779
Parent issue: CALCITE-3732

@wangxlong wangxlong requested review from chunweilei, danny0405 and hsyuan and removed request for chunweilei and hsyuan February 12, 2020 05:18
@hsyuan hsyuan changed the title [CALCITE-3779]Implement bitand scalar function [CALCITE-3779] Implement bitand scalar function Feb 19, 2020
@zinking
Copy link
Contributor

zinking commented Jul 1, 2020

looks good to me

Copy link
Member

@hsyuan hsyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wangxlong Can you rebase on latest master?

byte[] bytes1 = b1.getBytes();
byte[] result = new byte[b0.length()];

IntStream.range(0, bytes0.length).forEach(i -> {
Copy link
Contributor

@vlsi vlsi Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly speaking, a good old for loop would look better here, and it looks like streams add complexity without much benefits.

Suggested change
IntStream.range(0, bytes0.length).forEach(i -> {
for (int i = 0; i < bytes0.length; i++) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't agree more.

public static final SqlReturnTypeInference INTEGER_QUOTIENT_NULLABLE =
chain(ARG0_INTERVAL_NULLABLE, LEAST_RESTRICTIVE);


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refrain from altering non-related lines

}
byte[] bytes0 = b0.getBytes();
byte[] bytes1 = b1.getBytes();
byte[] result = new byte[b0.length()];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically speaking, b0.getBytes() clones the byte array, so there's no need to allocate new byte[...] for the result. I believe bytes0 would work just fine for the resulting array.

@wangxlong
Copy link
Contributor Author

Hi @hsyuan @vlsi I have rebased on latest master.

}

/** Bitwise function <code>BIT_AND</code> applied to binary and long values. */
public static ByteString bitAnd(ByteString b0, long b1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space before long

}
}

/** Implementor for bitwise scalar function. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space before scalar

*/
private static ByteString binaryOperator(
ByteString b0, long b1, BinaryOperator<Byte> bitOp) {
byte[] bytes0 = b0.getBytes();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final?

@wangxlong
Copy link
Contributor Author

@hsyuan Updated.

ByteString b0, long b1, BinaryOperator<Byte> bitOp) {
final byte[] bytes0 = b0.getBytes();

final byte[] result = new byte[bytes0.length];
Copy link
Contributor

@vlsi vlsi Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please remove result allocation and replace it with bytes0?
An alternative could be to use b0.byteAt(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Updated.

@wangxlong
Copy link
Contributor Author

@hsyuan @vlsi Hi, could you help me to review again, thanks.

@mihaibudiu
Copy link
Contributor

What's the status of this PR? It looks like it's close to being ready for merging.

@mihaibudiu
Copy link
Contributor

Superseded by #3942

@mihaibudiu mihaibudiu closed this Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants