Store NUMERIC columns as NUMERIC - #300
Draft
florinutz wants to merge 2 commits into
Draft
Conversation
`Numeric` compiled to `LONG` and `DECIMAL` to `DOUBLE`, both discarding the declared precision and scale. A `Decimal` written to such a column was truncated to an integer or rounded to a float, and SQLAlchemy re-applied the declared scale on the way out, so the value came back looking plausible. Render `NUMERIC(precision, scale)` instead, in DDL and in casts alike, and refuse at compile time to emit a column without a precision, which CrateDB does not store. A cast target may omit it, and still does. Hand bound `Decimal` values to the driver unconverted so it serializes them as strings, storing every digit. Reading remains bounded by the JSON responses, whose numbers arrive as floats.
…emy 2 `TYPES_MAP` had no entry for `numeric`, so a reflected column of that type resolved to an abstract placeholder and read back raw values. Map it, and its array form, to `NUMERIC`. `double` resolved to `DECIMAL` on SQLAlchemy releases without `DOUBLE`, and leaned on `DECIMAL` rendering the word `DOUBLE`. Give those releases a type that says so directly. Move the rule that a stored exact numeric needs a precision into the DDL compiler, which knows it is building a column definition. The type compiler renders what it is handed, so a cast target keeps the wider unbounded form, including inside `ARRAY`.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
sa.Numeric(10, 2)created aBIGINTcolumn.Decimal("1.25")went in andDecimal("1.00")cameback, with no error at any layer — SQLAlchemy re-applies the declared scale on read, so the truncated
integer returned looking like a plausible decimal.
Closes #292. Also delivers the write half of #163 — see the note on reads below.
Root cause
Two constants in
CrateTypeCompiler, added in 2018 when CrateDB had noNUMERICat all and mappingelsewhere was the only way to make the column creatable:
Both discard
type_.precisionandtype_.scale. CrateDB gainedNUMERICas a cast target in 4.4.0and storage for it in 5.9.0, and the mapping was never revisited.
docs/data-types.rsthad sincepublished it as the intended design.
There is a second, quieter half. The dialect leaves
supports_native_decimalat its default ofFalse, which makes SQLAlchemy'sNumeric.bind_processorreturnprocessors.to_float— so aDecimalwas already a float before the driver saw it. On aNUMERIC(38, 24)column:Fix
Render
NUMERIC(precision, scale), in DDL and in casts alike.NUMERICandDECIMALare the sametype in both SQLAlchemy and CrateDB, so
visit_DECIMALdefers tovisit_NUMERIC;sa.Numericandsa.NUMERICalready share that entry point through the generic compiler.Bind
Decimalvalues unconverted, via aNumericentry incolspecs, so the driver serializes themas strings — which it already does — and CrateDB stores every digit. Note this is not
supports_native_decimal = True: that flag also tells SQLAlchemy the DBAPI returnsDecimalobjectsand to skip result conversion, and this driver returns floats, so setting it would make
Numeric(asdecimal=True)hand back floats.Two points worth a maintainer's opinion:
CompileError. CrateDB rejects a bareNUMERICcolumn ("NUMERIC storage is only supported if precision and scale are specified"), sothere is nothing correct to emit. A cast target may omit the precision and still does, since the
server accepts
CAST(x AS NUMERIC). The alternatives were to emit bareNUMERICand let the serverrefuse it at execution, or to keep emitting
LONGwhen no precision is given — the latter wouldleave exactly this bug in place for anyone writing
sa.Numeric().Numericcolumns. Users on older servers get aworking (if lossy)
BIGINTtoday and would start seeing server errors. The repository documents nosupported-version floor, so I have not invented one — the requirement is stated in
CHANGES.mdandin the type map instead. Say the word if you would rather it degraded on older servers.
Reads are unchanged and remain bounded by the transport: responses are parsed as JSON, whose numbers
become floats, so a value returned from the database carries at most a double's digits. The server
sends the full value, so this is fixable in
crate-pythonrather than here; I will open an issuethere with the measurement, which also answers the question left open on crate/crate-python#652.
Tests
tests/numeric_test.py— DDL and cast rendering for all three spellings, precision without scale,a live round-trip of
Decimal("1.25"), and a live write of a value with more digits than a doubleholds.
DECIMALis deliberately tested with such a value: with1.25it would pass even whilerendering
DOUBLE, since a double holds1.25exactly.tests/create_table_test.py— the basic-types table now declares its exact-numeric columns with aprecision and expects
NUMERIC(10, 2).