Fix potential premature finish in Delta CDF function #17999
Conversation
| if (deltaLakePageSource.isFinished()) { | ||
| return FINISHED; | ||
| } else { | ||
| return TableFunctionProcessorState.Processed.produced(null); |
There was a problem hiding this comment.
The produced method has a requireNonNull check: https://github.com/trinodb/trino/blob/master/core/trino-spi/src/main/java/io/trino/spi/function/table/TableFunctionProcessorState.java#L89
| return FINISHED; | ||
| if (deltaLakePageSource.isFinished()) { | ||
| return FINISHED; | ||
| } else { |
There was a problem hiding this comment.
fmt expects line break before else
}
else {
There was a problem hiding this comment.
i will never get used to this format :|
|
Commit message title is too long: https://cbea.ms/git-commit/ |
|
The commit message should be something like "Fix XXX for Delta Lake". |
homar
force-pushed
the
homar/prevent_table_changes_from_skipping_read_data
branch
from
6b74505 to
48fd353
Compare
homar
changed the title
| while (!deltaLakePageSource.isFinished()) { | ||
| Page page = deltaLakePageSource.getNextPage(); | ||
| if (page == null) { | ||
| continue; |
There was a problem hiding this comment.
The idea behind CPS.getNextPage returning nullable is to break procesisng producing a page takes too long.
I think we should yield here. There are 3 ways we can go about it
- returning
TableFunctionProcessorState.Blocked.blocked(NOT_BLOCKED) - returning
TableFunctionProcessorState.Processed.produced(EMPTY_PAGE) - introducing new
TableFunctionProcessorState.Yielded
let's go with TableFunctionProcessorState.Processed.produced(EMPTY_PAGE) for a quick fix
and let's follow up with a PR introducing TableFunctionProcessorState.Yielded state
homar
force-pushed
the
homar/prevent_table_changes_from_skipping_read_data
branch
from
48fd353 to
5220eb9
Compare
| if (deltaLakePageSource.isFinished()) { | ||
| return FINISHED; | ||
| } | ||
| else { |
| if (deltaLakePageSource.isFinished()) { | ||
| return FINISHED; | ||
| } | ||
| else { |
|
i tried to understand whether this led me to this question: https://github.com/trinodb/trino/pull/13246/files#r1239083632 |
homar
force-pushed
the
homar/prevent_table_changes_from_skipping_read_data
branch
from
5220eb9 to
9ddb23b
Compare
Description
Additional context and related issues
Release notes
( ) This is not user-visible or 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: