Core: REST Scan Planning Task Implementation

[singhpk234]

🥞 Stacked PR

Please use this link for viewing incremental changes.

Current Stack status:

• Add integration with iceberg-core
  * Add Rest Model and Parsers [Merged]

About the change

[1] Add routes for the scan Planning API
[2] Adds RestTable and RestTableScan which can call the scan endpoint
[3] ScanIterable which uses ParallelIterable to fetch scan tasks

Testing

Added unit testing for the E2E req response loop

[amogh-jahagirdar]

I'm still going through things but I do believe this works at least from a correctness perspective. I still would need to give some more thought as to cancellation, client/server backpressure and how this would fit in for engines which immediately start task consumption/execution during planning (like Trino)

[singhpk234]

Thank you for the review, presently i just made E2E machinery work, with later parser changes, i think in addition to the points you mentioned, i was thinking of more also from the POV :

1. can server force client to call scan plan api ?
2. can server tell the client what should be interval between the fetch scan tasks ? (i think you refer that as back-pressure in above comment of yours)
3. I don't know but i keep coming to this feature [CORE] Support file filtering based on schema #4842 , it may be orthogonal
   but since for schema evolution, we certainly don't wanna send back data file objects which certainly don't have the column

[singhpk234]

Update : Working on refactoring this a bit more.

client/server backpressure and how this would fit in for engines which immediately start task consumption/execution during planning (like Trino)

My understanding was ParallelIterable could be helpful here as its aware of the both consumer and the producer,
and handle backpressure via yields

iceberg/core/src/main/java/org/apache/iceberg/util/ParallelIterable.java

Lines 200 to 205 in be03c99

	// If the consumer is processing records more slowly than the producers, the producers will
	// eventually fill the queue and yield, returning continuations. Continuations and new tasks
	// are started by checkTasks(). The check here prevents us from restarting continuations or
	// starting new tasks before the queue is emptied. Restarting too early would lead to tasks
	// yielding very quickly (CPU waste on scheduling).
	if (!queue.isEmpty()) {

Agree need to think this more thoroughly also from the server POV,

from my cursory reading of Trino source code (I am fairly new to it) https://github.com/trinodb/trino/blob/master/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitManager.java#L149 split generation and consuming it, should mostly work, as this seems like this is built in engine itself. for engine which needs all the splits computed first we have no choice but to consume everything.

[amogh-jahagirdar]

I think this is fine for now, we can address in a follow on. Not a blocker, just a nit of mine.

[nastra]

I've been mostly focusing on the tests and I've opened singhpk234#273 against your branch @singhpk234. The one remaining piece I haven't reviewed so far is the ScanTasksIterable (due to its complexity) but all of the other changes look good to me

[amogh-jahagirdar]

I checked out the latest code locally, I think we're very close just some final simplifications, published a PR
singhpk234#274 with my edits. Let me know what you think!

[singhpk234]

Thank you for the thorough feedback Amogh, i responded to the some of the feedbacks and addressed rest in my recent commit ! please let know what do you think considering what i had it in mind.

[geruh]

In the case of multiple planFile calls what do you think cancelling the previous plan?

TableScan scan = table.newScan();
scan.planFiles(); // Plan created
scan.planFiles(); // Plan created

[singhpk234]

Good callout, i think scan needs to be aware of the all the active plans, also i am thinking this will be problematic on concurrent request, let me sleep over it to think more !

[amogh-jahagirdar]

I don't think this is really something we need to worry about. If we put aside remote scan planning, and just take the client side planning case, these are 2 fundamentally different issuances of scanning work (e.g. in the client side case, we'd read manifests, sure on the second time maybe manifests are cached so we avoid the I/O but it's still doing the work and do the work of figuring out the tasks.). I think we just work from that semantic of the API.

[amogh-jahagirdar]

Thanks @singhpk234 , I think at this point this looks reasonable enough to get in, and I know other changes are depending on this, causing rebase challenges. I still feel like with some reasonable assumptions we can simplify ScanTaskIterable further but that's something we can discuss in follow on, and the current implementation looks reasonable without any races/deadlocks.

I'd say let's give it another day or so before merging for @nastra or @danielcweeks if they want to take another look.