-
Notifications
You must be signed in to change notification settings - Fork 427
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: Escape String for AS in external table #580
Conversation
Integration tests success for c304eebd5fd0747d911c8766538fabcbc853d934 |
@@ -113,7 +113,7 @@ func (tb *ExternalTableBuilder) Create() string { | |||
q.WriteString(fmt.Sprintf(` (`)) | |||
columnDefinitions := []string{} | |||
for _, columnDefinition := range tb.columns { | |||
columnDefinitions = append(columnDefinitions, fmt.Sprintf(`"%v" %v AS %v`, EscapeString(columnDefinition["name"]), EscapeString(columnDefinition["type"]), EscapeString(columnDefinition["as"]))) | |||
columnDefinitions = append(columnDefinitions, fmt.Sprintf(`"%v" %v AS %v`, EscapeString(columnDefinition["name"]), EscapeString(columnDefinition["type"]), columnDefinition["as"])) |
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.
I wonder if a more sustainable fix is to make it so that you can do columnDefinition.Get("name")
and it'll return behind the scenes EscapeString(columnDefinition["name"])
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.
Hmm columnDefinition
is a map of strings so there's no Get
method.
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 is now, but it doesn't have to be. I don't think you should make that change in the PR though
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.
Ohhh you were saying to change the structure of columnDefinitions
so that we could use Get
...
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.
What would the difference be from what I changed it to?
Addresses: #542
Test Plan
References