Skip to content

OpenAPI: Add min-rows-requested field to PlanTableScanRequest #14565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+13 −0
Open

OpenAPI: Add min-rows-requested field to PlanTableScanRequest #14565

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d08c6f
update IRC spec add minimum rows requested field to PlanTableScanRequest
ajreid21 Nov 12, 2025
396c942
update description
ajreid21 Nov 12, 2025
510aee5
fix
ajreid21 Nov 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions open-api/rest-catalog-open-api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1439,6 +1439,11 @@ class PlanTableScanRequest(BaseModel):
filter: Optional[Expression] = Field(
None, description='Expression used to filter the table data'
)
min_rows_requested: Optional[int] = Field(
Copy link
Contributor

@Fokko Fokko Nov 13, 2025

Choose a reason for hiding this comment

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

I find the name and description a bit confusing. For me, it is obvious that it might not return exactly the number of rows that are requested. For example, the table might be empty. I would much rather go with something that's analogue to SQL:

Suggested change
min_rows_requested: Optional[int] = Field(
limit: Optional[int] = Field(

Of course, it might still return more rows since the Parquet file contains more rows.

Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar Nov 13, 2025

Choose a reason for hiding this comment

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

I guess I look at things the other way since I feel like limit connotes a strict upper-bound (that is if there are sufficient matching rows) as how it means in SQL. Another idea: limit-hint?

Copy link
Member

@raulcd raulcd Nov 13, 2025

Choose a reason for hiding this comment

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

I agree with @Fokko , what about soft_limit?

Copy link
Contributor

@emkornfield emkornfield Nov 13, 2025

Choose a reason for hiding this comment

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

just to jump in on bike shedding how about max_rows_required I think it lacks the ambiguity if someone isn't familiar with SQL and I think my issue with and hopefully makes it clearer that this is in fact an upper bound?

Copy link
Contributor

@stevenzwu stevenzwu Nov 13, 2025

Choose a reason for hiding this comment

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

just to pile on. target-scan-task-number

Copy link
Contributor

@stevenzwu stevenzwu Nov 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

is it actually data row count or scan task count? I thought it is later. but if it is former, this config can be target-row-count.

This is similar to the table properties for write.target-file-size-bytes or read.split.target-size.

Copy link
Contributor

@stevenzwu stevenzwu Nov 13, 2025

Choose a reason for hiding this comment

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

if we are talking about fine grained control, maybe target-plan-size-bytes

Copy link
Contributor

@emkornfield emkornfield Nov 14, 2025

Choose a reason for hiding this comment

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

is it actually data row count or scan task count? I thought it is later. but if it is former, this config can be target-row-count.

I'll let @nastra confirm but I think it is a row count (as the original name suggests). target-row-count seems more ambiguous to me in terms of semantics.

Copy link
Contributor

@nastra nastra Nov 14, 2025

Choose a reason for hiding this comment

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

this is for rows and we're looking for a lower bound, hence why we're having the min in the name. We're open to better naming suggestions, but using something with limit in the name rather implies an upper bound, which wouldn't be correct here

Copy link
Contributor

@stevenzwu stevenzwu Nov 14, 2025

Choose a reason for hiding this comment

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

@nastra lower bound may not always be valid. e.g. maybe the table just don't have enough number of rows to satisfy the lower bound.

target is probably a good name for the desired plan size. Wondering why not using bytes (instead of rows) to express the target, similar to read.split.target-size?

Anyway, I would suggest target-plan-size-rows or target-plan-size-bytes.

None,
alias='min-rows-requested',
description='The minimum number of rows requested for the scan',
)
case_sensitive: Optional[bool] = Field(
True,
alias='case-sensitive',
Expand Down
5 changes: 5 additions & 0 deletions open-api/rest-catalog-open-api.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4402,6 +4402,11 @@ components:
description:
Expression used to filter the table data
$ref: '#/components/schemas/Expression'
min-rows-requested:
description:
The minimum number of rows requested for the scan
Copy link
Contributor

@singhpk234 singhpk234 Nov 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

[doubt] how is the server supposed to fulfill this request, when there are equality deletes present ?

Additional questions -

  • may be we should be explicit that its an hint ?
  • what would server do if its certain table doesn't have minimum number of row, 400 bad request ?
  • we might need to define iceberg scan api like we do for projection and filter so that E2E plumbing works

Copy link
Contributor Author

@ajreid21 ajreid21 Nov 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this is intended as a "hint" or "indicator" from the client to help the server not have to return more than is necessary. It is not required for the server to return that many results (as the result of the scan may not have that many rows, that's why it's named "min-rows-requested" and not "min-rows-required"). Also, it's not a max limit either and the server can return more than the requested number.

I'm open to different name and better description for this

Copy link
Contributor

@nastra nastra Nov 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What about The minimum number of rows requested for the scan. This is used as a hint to the server to not have to return more rows than necessary. It is not required for the server to return that many rows since the scan may not have that many rows. The server can also return more rows than requested

This should make it easier to understand that this is a hint and what the expectations are

Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar Nov 12, 2025

Choose a reason for hiding this comment

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

I think @nastra description is pretty reasonable, +1. This is fundamentally a hint to the server, and I would almost certainly not want to fail if a table doesn't have enough records.

Copy link
Contributor

@singhpk234 singhpk234 Nov 12, 2025

Choose a reason for hiding this comment

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

+1 from my end too ! it addresses my questions above

Copy link
Contributor Author

@ajreid21 ajreid21 Nov 12, 2025

Choose a reason for hiding this comment

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

Thanks, I updated the description.

type: integer
format: int64
case-sensitive:
description: Enables case sensitive field matching for filter and select
type: boolean
Expand Down