Use newTrinoTable in more places #24806
Conversation
ksobolew
force-pushed
the
kudi/new-trino-table-pk
branch
from
6a7f80f to
4afec8f
Compare
github-actions
bot
added
iceberg
ksobolew
force-pushed
the
kudi/new-trino-table-pk
branch
2 times, most recently
from
c6351db to
d7a837d
Compare
kokosing
left a comment
kokosing
left a comment
There was a problem hiding this comment.
Nice clean up. Let me know when it is ready to merge.
There was a problem hiding this comment.
Nice. I really like that these methods are now in sync.
There's one timeout in tests, but it looks like I'm racing with new uses that are creating conflicts... |
There was a problem hiding this comment.
IMO the primary-key variants should be dedicated methods with names making clear that they are to be used with MERGE-related tests.
I suggest newTrinoTableForMerge?
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
LGTM % comment about names vs new overloads
|
@chenjian2664 looks like you fixed the problem I wanted to fix with this :) I'll just keep the cleanups. |
Sometimes the new and old way were mixed in a single method. It also makes `createTestTableForWrites` reuse this method to avoid duplication.
ksobolew
force-pushed
the
kudi/new-trino-table-pk
branch
from
d7a837d to
2c350bd
Compare
ksobolew
changed the title
|
I eventually dropped the unification of the two methods, so only the last commit remains (and is amended a little). |
5 participants
Description
Sometimes the new and old way were mixed in a single method. It also makes
createTestTableForWritesreuse this method to avoid duplication.Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: