Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-10613] Implement BIT_AND for Beam SQL ZetaSQL dialect as CombineFn #12433

Closed
wants to merge 1 commit into from

Conversation

rehmanmuradali
Copy link
Contributor


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang SDK Dataflow Flink Samza Spark Twister2
Go Build Status --- Build Status --- Build Status ---
Java Build Status Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status Build Status
Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
--- Build Status ---
XLang Build Status --- Build Status --- Build Status ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@rehmanmuradali
Copy link
Contributor Author

rehmanmuradali commented Jul 31, 2020

R: @reuvenlax @kennknowles

@amaliujia
Copy link
Contributor

@Imfuyuwei

can you provide the NULL test case that failed this implementation before?

@Imfuyuwei
Copy link
Contributor

Yes, one of my previous commit has some tests related to NULL case.

a960772#diff-4f4ffa265fe666e99c37c346d50da67dR428-R434

@amaliujia
Copy link
Contributor

Thanks @Imfuyuwei

Yes, one of my previous commit has some tests related to NULL case.

a960772#diff-4f4ffa265fe666e99c37c346d50da67dR428-R434

@rehmanmuradali can you check this test case and make sure it passes before get this PR merged?

@rehmanmuradali
Copy link
Contributor Author

Thanks @Imfuyuwei

Yes, one of my previous commit has some tests related to NULL case.
a960772#diff-4f4ffa265fe666e99c37c346d50da67dR428-R434

@rehmanmuradali can you check this test case and make sure it passes before get this PR merged?

@amaliujia the test is failing. Seems like the issue is reported here BEAM-10379

@amaliujia
Copy link
Contributor

Yes. It seems an issue that under the surface. We have to fix that to make bit_and merged.

@rehmanmuradali
Copy link
Contributor Author

@amaliujia all aggregate functions implementation will be dependent on this ticket? I tried to change the NULL test case(initially bit_and) to bit_or and the issue reproduces.

@amaliujia
Copy link
Contributor

amaliujia commented Aug 5, 2020

My impression was bit_and crashed due to this special case while others didn't (or not tested). That was why bit_and was disabled.

Copy link
Member

@kennknowles kennknowles left a comment

Choose a reason for hiding this comment

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

Try this test case: SELECT bit_and(x) FROM (SELECT CAST(NULL AS int64) x);

Expected: NULL
Currently: 9223372036854775807

@stale
Copy link

stale bot commented Nov 26, 2020

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

@stale stale bot added the stale label Nov 26, 2020
@stale
Copy link

stale bot commented Dec 13, 2020

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@stale stale bot closed this Dec 13, 2020
@shehzaadn-vd
Copy link
Contributor

@ibzib @robinyqiu could you please help us reopen this PR?

cc: @sonam-vend

@robinyqiu robinyqiu reopened this Feb 23, 2021
@stale
Copy link

stale bot commented Feb 23, 2021

This pull request is no longer marked as stale.

@stale stale bot removed the stale label Feb 23, 2021
@ibzib
Copy link
Contributor

ibzib commented Sep 15, 2021

The nullness issue was fixed and BIT_AND enabled in #15174.

@ibzib ibzib closed this Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants