-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(cubeorchestrator): Increase VerifierOptions max_tables to 10M #10121
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10121 +/- ##
===========================================
+ Coverage 55.17% 78.11% +22.94%
===========================================
Files 213 457 +244
Lines 16904 91016 +74112
Branches 3335 3335
===========================================
+ Hits 9326 71093 +61767
- Misses 7100 19445 +12345
Partials 478 478
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ovr
left a comment
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.
LGTM, with minor nits
3120ca9 to
d04a455
Compare
And add parser tests
d04a455 to
302b16f
Compare
Native (rust) flatbuffer parser sometimes results in
FlatBuffer parsing errorfor large datasets, while JS parser works.The proble was because JS and Rust parsers use different FlatBuffer functions:
JS Parser (
packages/cubejs-cubestore-driver/codegen/http-message.ts:17-19):Rust Parser (
rust/cubeorchestrator/src/query_message_parser.rs:51-52):Why It Fails?
FlatBuffers verifier has default limits:
max_tables- maximum number of tables (1M)Client's data:
HttpRow+ eachHttpColumnValue= 1 tableThis PR updates the native parser with overridden VerifierOptions with increased
max_tables.Check List