-
Notifications
You must be signed in to change notification settings - Fork 324
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.
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
Implement Decimal support for Postgres backend #10216
Merged
Merged
Changes from all commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
3ad23f1
treat scale nothing as unspecifed
GregoryTravis 48464af
cast to decimal
GregoryTravis 6b694c7
float int biginteger
GregoryTravis 7951326
conversion failure ints
GregoryTravis 1e3da12
Merge branch 'develop' into wip/gmt/10162-d-col-conversions
GregoryTravis aba8d48
loss of decimal precision
GregoryTravis 145f9c0
precision loss for mixed column to float
GregoryTravis 8ce0dfe
mixed columns
GregoryTravis e41bf5b
loss of precision on inexact float conversion
GregoryTravis 1200cce
cleanup, reuse
GregoryTravis 8d94d5c
changelog
GregoryTravis 88e6714
Merge branch 'develop' into wip/gmt/10162-d-col-conversions
GregoryTravis 631820c
review
GregoryTravis d4ded95
no fits bd
GregoryTravis 24b265d
no warning on 0.1 conversion
GregoryTravis 3ac81b7
fmt
GregoryTravis a4b364b
big_decimal_fetcher
GregoryTravis 2553533
Merge branch 'develop' into wip/gmt/10213-read-pg
GregoryTravis c20d830
default fetcher and statement setting
GregoryTravis 0e6bd39
Merge branch 'develop' into wip/gmt/10213-read-pg
GregoryTravis 5d7f770
round-trip d
GregoryTravis 1f97af7
fix warning
GregoryTravis 49e50b3
Merge branch 'develop' into wip/gmt/10213-read-pg
GregoryTravis c11e3f6
expr +10
GregoryTravis b62dcea
double builder retype to bigdecimal
GregoryTravis 32fe09b
Use BD fetcher for underspecified postgres numeric column, not inferr…
GregoryTravis 643c231
Merge branch 'develop' into wip/gmt/10213-read-pg
GregoryTravis 2dd3927
fix tests
GregoryTravis f66889e
fix test
GregoryTravis 7e748e1
Merge branch 'develop' into wip/gmt/10213-read-pg
GregoryTravis 60c494f
cast_op_type
GregoryTravis 0eeba74
Merge branch 'develop' into wip/gmt/10213-read-pg
GregoryTravis b9afc85
no-ops for other dialects
GregoryTravis 111fad1
Types
GregoryTravis 4ae2902
sum + avg
GregoryTravis 4c4bc5b
avg + sum test
GregoryTravis b4f7115
fix test
GregoryTravis 4206905
update agg type inference test
GregoryTravis 358f89f
Merge branch 'develop' into wip/gmt/10213-read-pg
GregoryTravis 4815631
wip
GregoryTravis 2c87d7a
is_int8, stddev
GregoryTravis bea018a
more doc, overflow check
GregoryTravis 40f9b8d
fmt
GregoryTravis 357935b
Merge branch 'develop' into wip/gmt/10213-read-pg
GregoryTravis 8652753
finish round-trip test
GregoryTravis 6e7524f
wip
GregoryTravis 47dc410
Merge branch 'develop' into wip/gmt/10213-read-pg
GregoryTravis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am adding the cast here, at the SQL level, instead of using
DB_Column.cast
. This seems like the right thing to me, because we really just with that Postgres understood that a sum of integers is also an integer, and since it doesn't, we're helping it out here.And by doing it at this point, the regular type inference code will take care of the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the right place to call into this hook.
I'm wondering if this is the right thing to do with the type change though.While I understand we want to work with
long
and notBigInteger
in the in-memory backend, in a remote Postgres Database I'm not sure if the big-integer arithmetic will be the bottleneck. It may be in some cases I guess...The problem is - what if the result overflows? If the type of the Sum is the same as the type of the input, it is very easy to overflow it - just take two elements close to the maximum value representable in the input type and sum them.I think that is exactly the reason why the Postgres DB has these rules for 'promoting' the aggregate type to a larger type - to be able to hold a sum. I imagine it still has its limitations - a sum ofint4
column is promoted toint8
so if we sum 2^32 values of max int4 we could get it to overflow - but I assume it is rather rare to sum that many values, so it's probably 'good enough'.However as noted above - if we do not enlarge the type, we can get an overflow very easily with just 2 rows.Probably the problem is that our columns are mostlyint8
and so the only 'larger' type that we can promote to is thenumeric
(big-integer) type.Maybe we should use theint4
type more if our data fits only 32-bits? Then the aggregate would becomeint8
, still being a 'fast' number.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we still want to have this cast, we should at least ensure that we will not corrupt the data. We must add a test case where we have a column containing e.g. 2 rows with max
int8
value, and add them together. Let's see what happens :)I imagine if we get some kind of 'overflow' error - that will be OK. Ideally we should intercept this error and give the user a clear indication what happened and suggest that they can cast to a larger type (e.g.
numeric
) to get the result (paying the performance price).I think if we'd get a value that is modulo-truncated - that would be unacceptable as it would be a silent data corruption. But looking at a simple example:
I imagine we will likely just get an error, so it would just be good to intercept it and handle in a nice way for the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, I have wrote this comment before reading
Postgres_Dialect.enso
and I think I misunderstood the idea here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so do I understand correctly that the cast is needed to set the scale of the
NUMERIC
type to0
so that it is interpreted asBigInteger
and notBigDecimal
?I thought that we are casting to
int8
but apparently we are not, sorry for my confusing comments above...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, I guess it may be good to add such a test (Summing 2 rows that contain MAX INT64 values). It will be a good test verifying that our implementation is correct and does not cause data corruption in all kinds of DB backends (as I imagine how well the backends handle this may vary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this test.