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

fix: do not throw error if VALUE_DELIMITER is set on non-DELIMITED topic #4366

Merged
merged 1 commit into from
Jan 23, 2020

Conversation

agavra
Copy link
Contributor

@agavra agavra commented Jan 22, 2020

fixes #4200

Description

The VALUE_DELIMITER only makes sense in the context of DELIMITED format and today we throw an error if it is set in any other format. This PR removes that error and just ignores it if it is set on a non-delimited stream. This way:

  1. users can issue commands that take a delimited topic and turn it into another format
  2. if they go back to delimited (e.g. delimited->json->delimited) it will maintain the original source delimiter

Alternatively, we can just not pipe the delimiter and use the default if going from a non-delimited format back to delimited.

Testing done

  • updated unit tests
  • updated qtt
  • local end-to-end testing

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@agavra agavra requested a review from a team as a code owner January 22, 2020 23:44
@@ -261,6 +261,7 @@ private Format getValueFormat(final Sink sink) {
if (sink.getProperties().getValueDelimiter().isPresent()) {
return sink.getProperties().getValueDelimiter();
}

Copy link
Member

Choose a reason for hiding this comment

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

extra new line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's intentional - I thought it was cleaner to read with a new line than without

Copy link
Member

@stevenpyzhang stevenpyzhang left a comment

Choose a reason for hiding this comment

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

lgtm

@agavra agavra requested a review from a team January 23, 2020 00:04
@agavra agavra merged commit 2b59b8b into confluentinc:master Jan 23, 2020
@agavra agavra deleted the delimiter_fix branch January 23, 2020 00:38
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

Would a C* statement with DELIMITED set for a none DELIMIITED format no longer fail with this change? e.g.

CREATE TABLE FOO (...) WITH (VALUE_FORMAT='JSON', DELIMITER=',');

???

Allowing the above would be poor UX IMHO, as we're allowing the user to set something that doesn't make sense. If this no longer fails then I think we need a follow up PR to check for DELIMITER purely in CT/CS statements.

@agavra
Copy link
Contributor Author

agavra commented Jan 23, 2020

This is correct, I had thought about that (see the description) - it no longer fails, and I think that's OK. It does do something, it indicates that any DELIMITED format down the line will use this delimiter. The alternative would be to have the delimiter change when going between formats and back to delimited. Thoughts?

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.

Converting DELIMITED stream to another format fails when VALUE_DELIMITER is specified
3 participants