Skip to content

Support BigDecimal raw value forward index; Support BigDecimal in many transforms and operators #8622

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.

Sign up for GitHub

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

Jump to bottom
Merged
Jackie-Jiang merged 4 commits into from
May 3, 2022
Merged

Support BigDecimal raw value forward index; Support BigDecimal in many transforms and operators #8622

Jackie-Jiang merged 4 commits into from
May 3, 2022

Conversation

@nizarhejazi
Copy link
Contributor

@nizarhejazi nizarhejazi commented May 2, 2022 •
edited
Loading

  • Support BigDecimal raw value forward index
    • No dictionary group by
    • No dictionary distinct only, distinct w/ order by
    • etc.
  • Support BIG_DECIMAL in:
    • CASE statement.
    • CAST transform.
    • Groovy transform.
    • Addition, Subtraction, Multiplication, and Division.
    • abs(), ceil(), and floor() math functions.
    • Greatest transform.
    • Least transform.
    • EQUAL, NOT_EQUAL, IN, NOT_IN, RANGE predicates.
    • NoDictionary based EQUAL, NOT_EQUAL and IN predicates.
    • Binary operators (=, !=, >=, >, <=, <).
  • ~18 unit tests
  • Document the reason for choosing specific data structures for BigDecimal (TreeSet vs. HashSet)

feature, release-notes

@codecov-commenter
Copy link

codecov-commenter commented May 2, 2022 •
edited
Loading

Codecov Report

Merging #8622 (88d8a63) into master (b5690a7) will decrease coverage by 44.92%.
The diff coverage is 12.12%.

❗ Current head 88d8a63 differs from pull request most recent head b46c183. Consider uploading reports for the commit b46c183 to get more accurate results

@@              Coverage Diff              @@
##             master    #8622       +/-   ##
=============================================
- Coverage     70.47%   25.55%   -44.93%     
=============================================
  Files          1703     1694        -9     
  Lines         89569    89707      +138     
  Branches      13564    13625       +61     
=============================================
- Hits          63123    22923    -40200     
- Misses        21990    64558    +42568     
+ Partials       4456     2226     -2230
Flag Coverage Δ
integration1 ?
integration2 25.55% <12.12%> (+0.04%) ⬆️
unittests1 ?
unittests2 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...e/operator/dociditerators/SVScanDocIdIterator.java 77.04% <0.00%> (-9.16%) ⬇️
...edicate/BaseDictionaryBasedPredicateEvaluator.java 29.62% <0.00%> (-20.38%) ⬇️
...predicate/BaseRawValueBasedPredicateEvaluator.java 5.88% <0.00%> (-80.69%) ⬇️
...ter/predicate/EqualsPredicateEvaluatorFactory.java 65.00% <0.00%> (-18.34%) ⬇️
.../filter/predicate/InPredicateEvaluatorFactory.java 46.07% <0.00%> (-32.19%) ⬇️
.../predicate/NotEqualsPredicateEvaluatorFactory.java 47.29% <0.00%> (-30.65%) ⬇️
...lter/predicate/NotInPredicateEvaluatorFactory.java 50.45% <0.00%> (-28.76%) ⬇️
...lter/predicate/RangePredicateEvaluatorFactory.java 65.91% <0.00%> (-23.84%) ⬇️
...ator/transform/function/BaseTransformFunction.java 7.85% <0.00%> (-31.31%) ⬇️
...form/function/BinaryOperatorTransformFunction.java 20.61% <0.00%> (-23.83%) ⬇️
... and 1292 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5690a7...b46c183. Read the comment docs.

Jackie-Jiang
Jackie-Jiang reviewed May 2, 2022
View reviewed changes
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good, awesome work!

...n/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkSVForwardIndexWriter.java Outdated

public void putBigDecimal(BigDecimal value) {
_chunkBuffer.put(BigDecimalUtils.serialize(value));
_chunkDataOffset += BigDecimalUtils.byteSize(value);
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang May 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the serialized bytes length here instead of re-calculating the byte size again

@Jackie-Jiang Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes labels May 3, 2022
@Jackie-Jiang Jackie-Jiang merged commit 60ece7a into apache:master May 3, 2022
@nizarhejazi nizarhejazi deleted the bigdecimal2 branch May 3, 2022 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jackie-Jiang Jackie-Jiang Jackie-Jiang approved these changes

Assignees

No one assigned

Labels

feature release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nizarhejazi @codecov-commenter @Jackie-Jiang