Fix validation of decimal benchmark results - #399
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
misiugodfrey
left a comment
There was a problem hiding this comment.
I think the main fix to make sure the decimal types are preserved in the results files should work. However, some of the other additions here are - to my eye - either not necessary, or need further changes to refine our decimal comparisons.
Specifically, we are adding a new type.json file to track the calculated Presto types which could have different precision values than DuckDB - but we aren't using that information to refine our comparison, we are just using it to know if a type is a decimal, which is information we can already get from the expected_results file or from the results file itself now that the types are being written correctly.
This brings up that right now our decimal comparisons are absolute. Any differences in the calculated decimal types between Presto and DuckDB are not reflected in our evaluation - and that may be fine? If it is - then I think we can avoid the need to write a type.json file and keep things simpler. If it is not fine, then we can write the sidecar, but also incorporate the precision differences into our validation.
| column = frame.iloc[:, i] | ||
| frame.isetitem( | ||
| i, | ||
| column.map(lambda value: value if pd.isna(value) else Decimal(str(value))), |
There was a problem hiding this comment.
The actual Decimal objects here are created based on the value in the DataFrame, not the column_type; so the column_type passed in is only used to determine if a type is a decimal - the precision/scale values are not relevant as they will be determined by the DataFrame's value instead.
If that's the case (and we want to keep it that way, i.e. use the DataFrame as the source of truth) then it should not matter if we pass type info from DuckDB or Presto for the column_types as only their precision/scale should differ.
| results_dir.mkdir(parents=True, exist_ok=True) | ||
| parquet_path = results_dir / f"{query_id.lower()}.parquet" | ||
| df.to_parquet(parquet_path, index=False) | ||
| types_path = results_dir / f"{query_id.lower()}.types.json" |
There was a problem hiding this comment.
If the df is now being written with the decimal types preserved I'm not sure we need the additional types.json file? I understand that this provides us with the additional precision/scale information that Presto has calculated, but I don't think we use that information anywhere (Decimal comparisons are exact). If we think that information is necessary in the validation, then we should incorporate it into the comparison, but right now I think we are writing this new file to track information (is_decimal) that can be identified from the expected_results file, or from the Presto column types when written.
| if types_file.exists(): | ||
| return json.loads(types_file.read_text()) | ||
|
|
||
| inferred_types = [] |
There was a problem hiding this comment.
So from what I understand, we have three sources of type information:
- Presto types (now written to the
types.jsonfile). These should contain the decimal types and will have precison/scale values calculated by Presto - these may differ from the DuckDB prevision/scale specified in the expected_results files for computed expressions but should be the same for table columns). - DuckDB types. These are the types written in the expected_results files and will not have changed after this PR.
- Result file types. These are what we are writing when doing benchmarks and prior to this PR were being written without preserving the decimal types causing the comparison errors.
If I understand the issue correctly, the main problem was that the result file types were being written without preserving the decimal type, and therefore we were getting ordering errors because decimals were being compare lexicographically. However, I don't think that adding this file tracks new information (at least not in a way that it is used).
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def _get_column_types(result_file: Path) -> list[str] | None: |
There was a problem hiding this comment.
I think it's a little misleading to have a function called _get_column_types() return an empty list if none of the columns are decimals. If we do need this function to have this shape, then maybe we rename it?
|
|
||
| actual = pd.read_parquet(result_file) | ||
| expected = pd.read_parquet(expected_file) | ||
| actual_column_types = _get_column_types(result_file) |
There was a problem hiding this comment.
The actual_column_types we get here is only used in validate_query_results to determine if a column is decimal or not (the precision values are not taken into account). To that end, I think we can use the types from the expected_file instead of the results_file (and throw an error if the types do not align with the expected file). This eliminates the need for the sidecar file.
Summary
Fix benchmark validation for Presto DECIMAL result columns.
The Presto Python client returns DECIMAL values as strings. Benchmark results were therefore written as Parquet string columns, causing the validator to compare DECIMAL values lexicographically during
ORDER BYvalidation.Changes: