Skip to content

Move MERGE checks for nulls in non nullable columns to planner#13824

Merged
findepi merged 2 commits intotrinodb:masterfrom
homar:homar/add_check_for_nulls_in_nonnullable_columns_in_planner
Aug 25, 2022
Merged

Move MERGE checks for nulls in non nullable columns to planner#13824
findepi merged 2 commits intotrinodb:masterfrom
homar:homar/add_check_for_nulls_in_nonnullable_columns_in_planner

Conversation

@homar
Copy link
Copy Markdown
Member

@homar homar commented Aug 24, 2022

Description

Is this change a fix, improvement, new feature, refactoring, or other?
an improvement
Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)
core query engine
How would you describe this change to a non-technical end user or system administrator?
It should not affect end user directly

Related issues, pull requests, and links

Fixes: #13795

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# core
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Aug 24, 2022
@findepi
Copy link
Copy Markdown
Member

findepi commented Aug 24, 2022

Homar/add check for nulls in nonnullable columns in planner

@homar can you please give the PR a name?

@homar homar changed the title Homar/add check for nulls in nonnullable columns in planner Make merge respect non nullability of columns during planning Aug 24, 2022
@homar homar changed the title Make merge respect non nullability of columns during planning Move MERGE checks for nulls in non nullable columns to planner Aug 24, 2022
@homar homar requested review from alexjo2144 and findepi August 24, 2022 19:36
findepi
findepi approved these changes Aug 25, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/sql/planner/QueryPlanner.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Aug 25, 2022

Choose a reason for hiding this comment

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

coalesced -> checked

or, I'd perhaps simply reassign rewritten and keep rowBuilder.add(rewritten); after the if.

core/trino-main/src/main/java/io/trino/sql/planner/QueryPlanner.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Aug 25, 2022

Choose a reason for hiding this comment

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

newline before else

core/trino-main/src/main/java/io/trino/sql/planner/QueryPlanner.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Aug 25, 2022

Choose a reason for hiding this comment

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

belongs to the first commit

Copy link
Copy Markdown
Member Author

@homar homar Aug 25, 2022

Choose a reason for hiding this comment

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

strange that checkstyle didn't complain :/

Copy link
Copy Markdown
Member Author

@homar homar Aug 25, 2022

Choose a reason for hiding this comment

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

anyway there will be no else

Copy link
Copy Markdown
Member

@findepi findepi Aug 25, 2022

Choose a reason for hiding this comment

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

strange that checkstyle didn't complain :/

that's something we can try to improve. do you want to take a stab?

core/trino-main/src/main/java/io/trino/sql/planner/QueryPlanner.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Aug 25, 2022

Choose a reason for hiding this comment

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

redundant newline (fixed in the second commit, please amend the first one)

core/trino-main/src/main/java/io/trino/sql/planner/QueryPlanner.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Aug 25, 2022

Choose a reason for hiding this comment

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

redundant newline (fixed in the second commit, please amend the first one)

@homar homar force-pushed the homar/add_check_for_nulls_in_nonnullable_columns_in_planner branch from e4eba9d to 169f009 Compare August 25, 2022 09:32
@homar homar force-pushed the homar/add_check_for_nulls_in_nonnullable_columns_in_planner branch from 169f009 to 61a199b Compare August 25, 2022 09:36
@homar homar mentioned this pull request Aug 25, 2022
Closed
@homar
Copy link
Copy Markdown
Member Author

homar commented Aug 25, 2022

Failure is not related, I opened an issue for it: #13849

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Aug 25, 2022
@findepi
Copy link
Copy Markdown
Member

findepi commented Aug 25, 2022

Failure is not related, I opened an issue for it: #13849

Or #13778.

@findepi findepi merged commit 8757271 into trinodb:master Aug 25, 2022
@findepi
Copy link
Copy Markdown
Member

findepi commented Aug 25, 2022

Thanks!

@github-actions github-actions bot added this to the 394 milestone Aug 25, 2022
@colebow colebow mentioned this pull request Aug 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findepi findepi findepi approved these changes

@alexjo2144 alexjo2144 Awaiting requested review from alexjo2144

Assignees

No one assigned

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Milestone

394

Development

Successfully merging this pull request may close these issues.

Move checks for columns nullability for MERGE operation to Planner

2 participants

@homar @findepi