Skip to content

[CALCITE-6296] Support IS NULL in Arrow adapter#3858

Merged
asolimando merged 1 commit intoapache:mainfrom
timgrein:CALCITE-6296
Jul 19, 2024
Merged

[CALCITE-6296] Support IS NULL in Arrow adapter#3858
asolimando merged 1 commit intoapache:mainfrom
timgrein:CALCITE-6296

Conversation

@timgrein
Copy link
Contributor

Add the unary isnull Gandiva operator to ArrowTranslator.

Copy link
Member

@asolimando asolimando left a comment

Choose a reason for hiding this comment

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

LGTM

Edit: there seems to a problem with Sonar in CI which doesn't look like a transient failure, can you take a look @timgrein? Did you experience any issues locally?

@timgrein
Copy link
Contributor Author

Edit: there seems to a problem with Sonar in CI which doesn't look like a transient failure, can you take a look @timgrein? Did you experience any issues locally?

I couldn't reproduce it locally. I ran the following commands:

./gradlew clean ':arrow:test' --no-build-cache

and

./gradlew build. Any other commands you want to share, which might be helpful to investigate this? @asolimando

Both succeeded without the issue from CI, I'll check, if it appears again.

@timgrein timgrein requested review from asolimando and caicancai July 16, 2024 15:35
@caicancai
Copy link
Member

caicancai commented Jul 16, 2024

Edit: there seems to a problem with Sonar in CI which doesn't look like a transient failure, can you take a look @timgrein? Did you experience any issues locally?

I couldn't reproduce it locally. I ran the following commands:

./gradlew clean ':arrow:test' --no-build-cache

and

./gradlew build. Any other commands you want to share, which might be helpful to investigate this? @asolimando

Both succeeded without the issue from CI, I'll check, if it appears again.

2024-07-16 23-36-14屏幕截图

You can look at the cli log.
Integration/jenkins/pr-head may occasionally fail. If the log exception is not very strange, I think this will not affect the merge.

@timgrein
Copy link
Contributor Author

Edit: there seems to a problem with Sonar in CI which doesn't look like a transient failure, can you take a look @timgrein? Did you experience any issues locally?

I couldn't reproduce it locally. I ran the following commands:
./gradlew clean ':arrow:test' --no-build-cache
and
./gradlew build. Any other commands you want to share, which might be helpful to investigate this? @asolimando
Both succeeded without the issue from CI, I'll check, if it appears again.

2024-07-16 23-36-14屏幕截图

You can look at the cli log. Integration/jenkins/pr-head may occasionally fail. If the log exception is not very strange, I think this will not affect the merge.

Thanks for the context! Seems like it succeeded now.

Copy link
Member

@asolimando asolimando left a comment

Choose a reason for hiding this comment

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

Few minor changes emerged during review, let's address them, but they are minor and we are converging

@timgrein
Copy link
Contributor Author

Few minor changes emerged during review, let's address them, but they are minor and we are converging

Thanks for the detailed review @asolimando, already helped a lot! Addressed your changes, let me know, if there's something else.

@timgrein timgrein requested a review from asolimando July 17, 2024 10:32
@caicancai
Copy link
Member

Mostly looks good, just a few minor comments

Copy link
Member

@asolimando asolimando left a comment

Choose a reason for hiding this comment

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

LGTM, one final nitpick but it's optional, all other comments seem to have been addressed.

Copy link
Member

@caicancai caicancai left a comment

Choose a reason for hiding this comment

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

LGTM

@asolimando
Copy link
Member

If @NobiGo is fine as well with the current status of the PR, I will ask you @timgrein to squash everything into a single commit and I will then merge. Please wait for the final confirmation before squashing as it makes incremental reviewing way harder, thanks!

@timgrein timgrein requested a review from NobiGo July 18, 2024 16:19
@asolimando
Copy link
Member

@timgrein, can you please squash the commits into a single one, having "[CALCITE-6296] Support IS NULL in Arrow adapter" as commit message? After that I will merge, thanks!

@sonarqubecloud
Copy link

@asolimando asolimando merged commit e371b33 into apache:main Jul 19, 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.

4 participants