Skip to content
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

-Support for Source Codecs #2519

Merged

Conversation

umayr-codes
Copy link
Contributor

@umayr-codes umayr-codes commented Apr 18, 2023

Signed-off-by: umairofficial [email protected]

Description

  • InputCodec interface added in data-prepper-api package
  • JSONCodec moved from s3-source to parse-json-processor
  • CSVCodec moved from s3-source to csv-processor
  • NewlineCodec moved from s3-source to newline-codecs
  • Review comments incorporated

Issues Resolved

Resolves #1532

Check List

  • New functionality includes testing.
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: umairofficial <[email protected]>
@umayr-codes umayr-codes requested a review from a team as a code owner April 18, 2023 09:43
@@ -61,22 +62,12 @@ private void parseBufferedReader(final BufferedReader reader, final Consumer<Rec
}

MappingIterator<Map<String, String>> parsingIterator = mapper.readerFor(Map.class).with(schema).readValues(reader);
boolean hasNextValue;
Copy link
Member

Choose a reason for hiding this comment

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

@umayr-codes , You have reverted some changes in your most recent rebase. See this commit: bc75494

I believe you can resolve this issue by changing all the code from this line through the end of this file back. Basically, we should not see any diffs after line 41.

You may also need to check the imports above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its done David!

@@ -282,7 +280,7 @@ void test_when_manualHeaderTooManyColumns_then_omitsExtraColumnsOnParsedEvents(f

@ParameterizedTest
@ValueSource(ints = {2, 10, 100})
void test_when_autoDetectHeaderWrongNumberColumnsAndJaggedRows_then_skipsRemainingRows(final int numberOfRows) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Similar to my comment above about bc75494, this PR currently is reverting changes in main that need to remain.

You should revert back the changes starting at this line. There should be no diffs from this point forward.

Also, please check the imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Thanks for pointing out.

Signed-off-by: umairofficial <[email protected]>
Copy link
Member

@dlvenable dlvenable left a comment

Choose a reason for hiding this comment

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

Removing accidental approval

Copy link
Member

@dlvenable dlvenable left a comment

Choose a reason for hiding this comment

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

Removing accidental approval

@dlvenable dlvenable self-assigned this Apr 18, 2023
Copy link
Collaborator

@kkondaka kkondaka left a comment

Choose a reason for hiding this comment

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

Looks good.

@ashoktelukuntla
Copy link
Contributor

Looks good Umair

Copy link
Member

@dlvenable dlvenable left a comment

Choose a reason for hiding this comment

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

Thanks @umayr-codes ! We should be able to merge once all the checks pass.

@dlvenable dlvenable merged commit 5dadde4 into opensearch-project:main Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support generic parsers/codecs
5 participants