-
Notifications
You must be signed in to change notification settings - Fork 334
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
chore: change binary array type from LargeBinaryArray to BinaryArray #3924
chore: change binary array type from LargeBinaryArray to BinaryArray #3924
Conversation
To verify the read parquet files functionality the following steps have been taken (many thanks to @evenyag)
|
@etolbakov May I ask what do you think this patch is still a draft? AFAICS it's ready for review. |
Thanks! This is a good news. I'm also considering constructing a test case that uses the
The second way is more readable. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3924 +/- ##
==========================================
- Coverage 85.40% 85.13% -0.28%
==========================================
Files 978 978
Lines 169100 169283 +183
==========================================
- Hits 144427 144124 -303
- Misses 24673 25159 +486 |
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's almost done. The only issue is that we should cast the BinaryArray to a LargeBinaryArray.
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. Thanks!
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
Thank you! |
…reptimeTeam#3924) * chore: change binary array type from LargeBinaryArray to BinaryArray * fix: adjust try_into_vector logic * fix: apply CR suggestions, add tests * chore: fix failing test * chore: fix integration test * chore: adjust the assertions according to changed implementation * chore: add a test with LargeBinary type * chore: apply CR suggestions * chore: simplify tests
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
#3913
What's changed and what's your intention?
Checklist