s3select - Add tests (and some optional funcationality for them) #7222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alphaprinz
force-pushed
the
s3select
branch
4 times, most recently
from
b3307a8
to
bd017ad
Compare
alphaprinz
changed the title
dannyzaken
reviewed
Mar 19, 2023
| ] | ||
| `; | ||
|
|
||
| class S3SelectStream extends Transform { |
There was a problem hiding this comment.
Why aren't you using the S3SelectStream in util/s3select?
There was a problem hiding this comment.
The "real" S3SelectStream in util/s3select also has the AWS chunk enconding.
If we want to reuse it we need to make the encoding optional (eg add parameter/move to a different stream).
There was a problem hiding this comment.
Added a comment to explain rationale of the new class.
shirady
reviewed
Apr 2, 2023
src/test/system_tests/ceph_s3_tests/s3-tests-lists/s3_tests_pending_list.txt
Outdated
Show resolved
Hide resolved
alphaprinz
force-pushed
the
s3select
branch
3 times, most recently
from
02a5fb0
to
d1ec6ff
Compare
shirady
reviewed
Apr 9, 2023
shirady
reviewed
Apr 9, 2023
shirady
reviewed
Apr 16, 2023
docs/dev_guide/ceph_s3_tests/ceph_s3_tests_pending_list_status.md
Outdated
Show resolved
Hide resolved
shirady
approved these changes
Apr 16, 2023
There was a problem hiding this comment.
LGTM
@alphaprinz, Thank you for adding comments with detailed explanations!
2 (last) comments:
- @guymguym, would you please review tests
double pipe closeandasync loop error? - @alphaprinz, would please organize the PR description?
For example:
- add testing instructions for running the file you created
- add the gap of parquet file (it can be a Jira link)
- add to the changes updating to
test_s3select.js
alphaprinz
force-pushed
the
s3select
branch
2 times, most recently
from
ea9ee81
to
6d349c2
Compare
dannyzaken
approved these changes
Apr 18, 2023
Implement Stats message. Send End message on errors. Add FileHeaderInfo=IGNORE. Add unit tests. Uncomment most ceph s3select tests from pending list. Add a manual patch for ceph tests code so teardown will delete bucket created by test. Signed-off-by: Amit Prinz Setter <aprinzse@ibm.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explain the changes
First commit adds some optional functionality which is tested in ceph s3select tests.
Second commit adds tests.
Issues: Fixed #xxx / Gap #xxx
1a. End message on error.
1b. Stats message on completion.
1c. FileHeaderInfo=IGNORE (it's a different field in s3selct csv_definitions struct).
New unit tests.
Most ceph tests related to s3select were removed from pending list.
(Remaining tests have bugs in tests)
A manual fix is applied to test code so that bucket name would conform to bucket name pattern.
After fix teardown properly deletes the bucket created by test.
Without fix only one test passes. The second test fails when it tries to create an already existing bucket.
Testing Instructions: