[FLINK-3859] [table] Add BigDecimal/BigInteger support to Table API #2088
Conversation
twalthr
force-pushed
the
TableApiBigDecBigIntIntegration
branch
2 times, most recently
from
d0eee42
to
812febc
Compare
| s"'${genExpr.resultType}'") | ||
| } | ||
|
|
||
| def requireNumericOrComparable(genExpr: GeneratedExpression) = |
There was a problem hiding this comment.
Rename to requireComparable()?
|
Thanks for the PR, @twalthr! Looks mostly good. I found a few cases that needs to be fix. We also should think about how to handle SQL |
|
Think we also need aggregation functions for @twalthr I can implement the aggregation functions, if you want to. |
|
@fhueske Thanks for reviewing my code. I totally forgot to test the comparisions. I will fix the issues and see what I can do for fixed precision / scale cases. Would be great if you could implement the corresponding aggregation functions. |
|
Hi @twalthr, I implemented the aggregation functions and pushed the commit to a branch in my repository: https://github.com/fhueske/flink/tree/tableDecimal |
twalthr
force-pushed
the
TableApiBigDecBigIntIntegration
branch
2 times, most recently
from
352674d
to
92d2ce8
Compare
|
@fhueske I have updated my PR. Now comparisons should work. Once the build succeeded, I will add your commits. |
|
Thanks for the update @twalthr. The changes look good. Should be good to merge after the aggregators commit is added. |
twalthr
force-pushed
the
TableApiBigDecBigIntIntegration
branch
from
92d2ce8
to
44bdf26
Compare
|
Merging |
|
Won't merge this PR at this point. A few Travis builds timed out and I found that the added expression tests increase build time by about one minute due to the code-gen compilation overhead. @twalthr, do you think we can adapt the tests to batch several expressions into a single class with multiple methods to invoke the compiler less frequently? |
|
I also already recognized the problem of long running tests. I think will rework the test base again. So that there is one compilation per unit test. |
twalthr
force-pushed
the
TableApiBigDecBigIntIntegration
branch
from
077fedb
to
ef4d5ca
Compare
|
@fhueske I have changed the |
|
If there are no objections, I would merge this later today... |
|
Thanks for refactoring the tests @twalthr! LGTM |
|
Thanks Fabian. Merging... |
mvn clean verifyhas been executed successfully locally or a Travis build has passedThis PR introduces the BigDecimal type (and thus also BigInteger) to the Table API. Basic arithmetic operations as well as ABS, FLOOR and CEIL are now possible. I fixed several bugs and refactored some code parts (e.g. I introduced an ExpressionTestBase).