Conversation
|
Neil, If there isn't a customer waiting on this, I suggest we pause this change. See my comments in PR 1823. Sundar Mudupalli |
|
/gcbrun |
| decimal.Decimal(str(_)) | ||
| if not pandas.isna(_) and isinstance(_, (int, float, str)) | ||
| else _ | ||
| ) |
There was a problem hiding this comment.
Neil,
I want to review this code a bit carefully - specifically lines 154 to 159. Looking at the code, it appears that for a decimal type pandas could be returning a float and you may be converting it to decimal. That could result in a loss of precision and create the same issue that we had. There are a couple of ways to avoid this - materialize the results in pyArrow - which will give you the real decimal types and you don't have a conversion and/or specify that ibis use the PyArrow data types instead (available in Pandas 2.x).
On the other hand, you may have come up with a good workaround. I want to test it with different data values to ensure I am not missing something. Apologies, I could not get to it today.
Sundar Mudupalli
There was a problem hiding this comment.
Okay, no problem. In case it helps:
- The change described in item 3 in the PR description is intended to prevent exactly what you describe, to stop Pandas coercing decimals to floats.
- Reviewing SQL for
pso_data_validator.dvt_large_decimalswill show you the types of values we already test with. Previously the test was flawed but I believe it can now be trusted.
There was a problem hiding this comment.
I asked an AI agent for a second opinion on my work and it flagged a potential further issue that probably requires a new issue:
Oracle can still deliver floats. By default,
python-oracledbreturnsNUMBER(p,s>0)asfloat(fetch_decimalsdefaults toFalse). So an Oracle decimal key with a fractional part and more than ~15 significant digits loses precision inside the driver, unrelated to Pandas.coerce_float=Falsecan't fix that.
So we probably need a new test table with fractional primary key values and a new issue. I would prefer for that to not be combined with this fix which I believe is independent.
|
/gcbrun |
Description of changes
This PR fixes three issues with random row validation issues for large decimal primary keys
1. Preserve Decimal PK Types in Random Row Filters
Added _prepare_pk_filter_values in
data_validation/data_validation.pyto coerce sampled PK values intodecimal.Decimalwhen the column type isdecimal, preventing Ibisint64bounds errors when drivers return large integers.2. Support Large Decimal Literals in BigQuery
Implemented a custom
format_literaltranslator for BigQuery inthird_party/ibis/ibis_bigquery/registry.pythat formats decimals as fixed-point strings (avoiding invalid scientific notation) and emits BIGNUMERIC when integer precision exceeds standard NUMERIC limits.3. Prevent Decimal-to-Float Precision Loss in SQLAlchemy Backends
Overrode
BaseAlchemyBackend.fetch_from_cursor(_dvt_fetch_from_cursorinthird_party/ibis/ibis_addon/operations.py) withcoerce_float=Falseto prevent pandas from castingDecimalobjects tofloat64and losing digits beyond 15-17 places.Issues to be closed
Closes #1822
Checklist
CONTRIBUTINGGuide.tests/local_check.shscript)