-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: indicate header columns in source descriptions #8475
Conversation
@@ -31,22 +31,26 @@ | |||
|
|||
public enum FieldType { | |||
SYSTEM, // To be removed in the future. 0.9 saw this value no longer used. |
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.
This can be removed, right? I don't see being used either.
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.
The purpose of keeping this was so that pre-0.9 servers could still work with newer CLIs. I'm not sure how many people are still using 0.9, but I think this is easy enough to just keep anyway. This does raise a good point though - the changes in this PR would make HEADERS require the newest CLI. Are we okay with that?
|
||
@JsonCreator | ||
public FieldInfo( | ||
@JsonProperty(value = "name", required = true) final String name, | ||
@JsonProperty(value = "schema", required = true) final SchemaInfo schema, | ||
@JsonProperty("fieldType") final Optional<FieldType> type | ||
@JsonProperty("fieldType") final Optional<FieldType> type, | ||
@JsonProperty("headerKey") final Optional<String> headerKey |
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.
Seems this field is optional in different places. What if you create another constructor that defaults to Optional.empty()?
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.
Done
|
||
Queries that read from this TABLE | ||
----------------------------------- | ||
readId (ERROR) : read query |
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.
is this ERROR ok?
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.
Yeah, all the source description tests have query errors to test printing errors
final String headerType; | ||
if (field.getHeaderKey().isPresent()) { | ||
headerType = "(header('" + field.getHeaderKey().get() + "'))"; | ||
} else { | ||
headerType = "(headers)"; | ||
} |
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.
Tip
final String headerType = field.getHeaderKey()
.map(k -> "(header('" + k + "'))")
.orElse(() -> "(headers)");
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.
Done
Description
DESCRIBE
statements will indicate which columns are headers, for exampleTesting done
Manual, unit, console tests
Reviewer checklist