Skip to content

Add redirection awareness for MERGE#14055

Merged
phd3 merged 2 commits intotrinodb:masterfrom
findinpath:merge-redirection-awareness
Sep 13, 2022
Merged

Add redirection awareness for MERGE#14055
phd3 merged 2 commits intotrinodb:masterfrom
findinpath:merge-redirection-awareness

Conversation

@findinpath
Copy link
Copy Markdown
Contributor

@findinpath findinpath commented Sep 8, 2022

Description

Add redirection awareness for the task:

  • MERGE INTO target USING source

Is this change a fix, improvement, new feature, refactoring, or other?

This is a fix.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

This is a core engine change affecting mainly hive/iceberg connectors.

How would you describe this change to a non-technical end user or system administrator?

In the context of dealing with a shared Hive Metastore Service where tables of different types can co-exist in the metastore, it is helpful for the end-user to perform automatically table redirections when the user is in the context of a hive schema, but is referencing an iceberg table.

Related issues, pull requests, and links

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

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

# Core
* Add redirection awareness for `MERGE` operation. ({issue}`14055`)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: you can put ON on the previous like, just like you would usually do with a JOIN

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

checkstyle should error here

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i see it didn't, but we have a rule that disallows whitespace before ), right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think you are hinting at airlift/airbase#323
I can convert the text block to a string concat construct.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you are hinting at airlift/airbase#323

no

I can convert the text block to a string concat construct.

no need to.

the )) should be on the previous line though

@findinpath findinpath force-pushed the merge-redirection-awareness branch from f68bb4c to 2ebb18c Compare September 8, 2022 14:19
@findinpath
Copy link
Copy Markdown
Contributor Author

CI failure on suite-2 is a timeout failure.

@findinpath findinpath requested a review from findepi September 9, 2022 04:46
@findinpath findinpath force-pushed the merge-redirection-awareness branch from 6970018 to 71cb948 Compare September 12, 2022 04:11
@findinpath findinpath requested a review from findepi September 12, 2022 04:11
@findinpath findinpath force-pushed the merge-redirection-awareness branch from 71cb948 to 99c3ae0 Compare September 12, 2022 07:59
@findinpath findinpath force-pushed the merge-redirection-awareness branch from 99c3ae0 to 67be5a2 Compare September 12, 2022 08:09
@findinpath findinpath requested a review from phd3 September 12, 2022 18:57
Copy link
Copy Markdown
Member

@phd3 phd3 left a comment

Choose a reason for hiding this comment

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

#14055 (comment) seems unaddressed, but lgtm. will merge if @findepi doesn't feel too strongly.

@findepi
Copy link
Copy Markdown
Member

findepi commented Sep 13, 2022

i don't feel strongly about that one, thank you @phd3

@phd3 phd3 merged commit 2ecee39 into trinodb:master Sep 13, 2022
@phd3
Copy link
Copy Markdown
Member

phd3 commented Sep 13, 2022

Merged, thanks!

@github-actions github-actions bot added this to the 396 milestone Sep 13, 2022
@colebow
Copy link
Copy Markdown
Member

colebow commented Sep 13, 2022

I'm not sure the proposed release note works here... if the template says we're fixing something, what are we fixing / what impact will the end user see? And even if it's a change to the core engine, if it only affects Hive/Iceberg, should we just list the note in those sections?

cc @findepi @phd3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants