Repository navigation
[Python] Keep Nexmark Query 1 bid prices as integers - #40548
Conversation
Query 1 multiplied the bid price by a float, so the converted Bid could not be encoded by BidCoder, which writes the price as a varint. Use the same integer arithmetic as the Java Query 1. Fixes apache#24678
|
Assigning reviewers: R: @tvalentyn for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Thanks for the contribution. The fix sounds valid, however Nexmark benchmarks has been unmaintained for long and old issues may be out-of-date. If still interested:
|
|
Thanks, makes sense. I removed the unit test and re-enabled Query 1 in I ran the suite locally the same way that workflow does: Java Nexmark (
On master, Query 1 fails with: One thing I noticed: the Python launcher logs "Discarding unparseable args" for |
Abacn
left a comment
There was a problem hiding this comment.
LGTM after resolving merge conflict
| * (Python) `Duration` built from float seconds now rounds to the nearest microsecond instead of truncating, which could lose a microsecond ([#40263](https://github.com/apache/beam/issues/40263)). | ||
| * (Python) `ReadFromText` with `escapechar` no longer skips a delimiter that directly follows an escaped delimiter, which merged two records into one ([#40459](https://github.com/apache/beam/issues/40459)). | ||
| * (Python) DataFrame API: `agg` with a list that includes `'count'` or `'size'` now labels those rows `count`/`size` instead of `sum` ([#40504](https://github.com/apache/beam/issues/40504)). | ||
| * (Python) Nexmark Query 1 now keeps bid prices as integers, so its output can be encoded by `BidCoder` ([#24678](https://github.com/apache/beam/issues/24678)). |
There was a problem hiding this comment.
We can skip CHANGES.md as this is a test-only change
There was a problem hiding this comment.
I mean we don't add items to CHANGES.md for test-only change
There was a problem hiding this comment.
Done, removed the entry.
…integer-price # Conflicts: # CHANGES.md
|
Thanks! Merged master and resolved the CHANGES.md conflict. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40548 +/- ##
=============================================
+ Coverage 56.16% 79.90% +23.73%
=============================================
Files 1124 488 -636
Lines 178336 82384 -95952
Branches 1489 0 -1489
=============================================
- Hits 100170 65829 -34341
+ Misses 75645 16555 -59090
+ Partials 2521 0 -2521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes #24678
Python Nexmark Query 1 converts bid prices with
bid.price * 0.89, which turns the price into a float.BidCoderwrites the price withVarIntCoderImpl, so encoding the converted bid fails with:The Java version of the query does integer math,
(bid.price * 89) / 100. This change does the same in Python with(bid.price * 89) // 100, so the result is still an int and matches the Java output.This also re-enables Query 1 in the Python Nexmark Direct postcommit, which had excluded it because of this issue.
Testing: ran the Nexmark suite locally the same way the Direct postcommit does (Java Nexmark to generate 100,000 events, then the Python launcher on DirectRunner). Query 1 fails on master with the error above and passes with this change; queries 0 and 2 give the same results with and without it.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123orfixes #456).CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.