fix: raise PartialResultError when server truncates a response#266
fix: raise PartialResultError when server truncates a response#266jeremy-xxx wants to merge 3 commits into
Conversation
DataMatrices.partial_result signals that the server's maximum response size was exceeded and not all matching data was returned, even for unbounded requests. data_read and valuematrix_read previously ignored this flag, silently handing back incomplete data. Both now raise PartialResultError so callers notice truncation instead of assuming they received everything.
totonga
left a comment
totonga
left a comment
There was a problem hiding this comment.
The main issue about integrtaing changes like this is breaking backwards compatibility of the library.
Idea: Its not an error to ignore partial results in most of the use cases because it just tells your query was bad. "Please add a condition".
Chunking on rows is not that easy because missing orderby or added data might break your use case anyway. There is no fixed window in ASAM ODS API.
So at the end it should only happen in case of pandas convertion if a parameter tells it should throw because it is hidden afterwards.
It might also be a good idea to add it to the dataframe attributes as info to allow detection in a following step.
| response = self.ods_post_request("data-read", select_statement) | ||
| return_value = ods.DataMatrices() | ||
| return_value.ParseFromString(response.content) | ||
| ConI._check_partial_result(return_value) |
There was a problem hiding this comment.
Is is no good idea to raise an error in data_read itself.
data_read is the direct ASAM ODS warpper that is just returning DataMatrices itself.
The ASAM ODS interface defines that it might be a good idea to return partial result and allow the client to react on it.
if an raise might be integrated it must happen in the places where the partial result is hidden.
This happens when the DataMatrices are converted to pandas dataframes.
| response = self.ods_post_request("data-read", select_statement) | ||
| return_value = ods.DataMatrices() | ||
| return_value.ParseFromString(response.content) | ||
| ConI._check_partial_result(return_value) |
There was a problem hiding this comment.
Another very important aspect is backwards compatibility.
This means current behavior can only be changed using a switch.
For some usecases it might be O.K. to get a partial result.
There are two main usecases:
- Asking for all rows does not fit into max size of protobuf message. A limit is added but not every application cares. Getting 10k and displaying some the user determines that he needs to add a query condition.
- local_column values do not fit into protobuf message. So it is cut. This might be a place to eventually throw because client is not aware of doing something wrong.
However to not break existing use cases it must be a parameter that enables thrwo and is only used before converting to pandas dataframe. (Do you share this approach?)
There was a problem hiding this comment.
Thanks for the detailed feedback — yes, I share this approach. You're right that
data_read is a thin wrapper where the caller can still see the flag and react to
it, and that a partial result is a legitimate outcome for many queries. The real
problem is only that the flag gets lost in the pandas conversion.
I've updated the PR accordingly (49b04ab):
- data_read / valuematrix_read are reverted to their original behavior — no raise,
DataMatrices returned as-is. - to_pandas now always preserves the flag as df.attrs["partial_result"] (also on
the empty-result early returns), so it can be detected in a following step. - A new opt-in parameter raise_on_partial_result (default False) raises
PartialResultError from to_pandas for callers who want a hard failure — e.g.
the local_column values case you mentioned, where the client isn't aware of
doing something wrong. Default behavior is unchanged. - query / query_data pick the parameter up through their existing **kwargs
passthrough to to_pandas, so no signature changes there.
Happy to adjust naming or move things around if you'd prefer it structured
differently.
Per review: data_read/valuematrix_read are thin ASAM ODS wrappers that return DataMatrices as-is, so callers can inspect partial_result there themselves and ignoring it is a valid choice. The flag only becomes hidden when converting to pandas. to_pandas now preserves it as df.attrs["partial_result"] and offers an opt-in raise_on_partial_result parameter (default False, keeping existing behavior) that raises PartialResultError. query/query_data forward the parameter via their existing **kwargs passthrough.
2 participants
Summary
DataMatrices.partial_resultsignals the server hit its max response size anddid not return all matching data — even for unbounded (
values_limit=0) requests.ConI.data_read()andConI.valuematrix_read()previously ignored this flag,silently returning truncated data to callers.
PartialResultErrorwhenpartial_resultis set, so truncationis surfaced instead of silently swallowed.
query/query_data/data_read_jaquelinherit the check since they route through
data_read.Test plan
uv run pytest tests/— 402 passed (includes existing tests against thelive demo server, confirming no false positives on normal bounded queries)
uv run ruff check/ruff format --check— cleanuv run mypy src/— cleantests/test_con_i_partial_result.pycoveringpartial_result=True/Falsefor both
data_readandvaluematrix_read