Skip to content

Conversation

@gengliangwang
Copy link
Member

What changes were proposed in this pull request?

Sometimes users forgot to add single quotes on the map key string literal, for example map_col[a]. In such a case, the Analyzer will throw an exception:

[MISSING_COLUMN] Column 'struct.a' does not exist. Did you mean one of the following? ...

We can improve this message by saying that the user should append single quotes if the map key is a string literal.

[UNRESOLVED_MAP_KEY] Cannot resolve column 'a' as a map key. If the key is a string literal, please add single quotes around it. Otherwise, did you mean one of the following column(s)? ...

Why are the changes needed?

Error message improvement

Does this PR introduce any user-facing change?

Yes but trivial, an improvement on the error message of unresolved map key column

How was this patch tested?

New UT

@gengliangwang
Copy link
Member Author

cc @srielau as well

}

// If an attribute can't be resolved as a map key of string type, either the key should be
// surrounded with single quotes, or there is a typo in the attribute name.
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to match element_at and try_element_at here. But before the functions are not resolved yet in the rule ResolveFunctions, the error MISSING_COLUMN is thrown.
Checking the unresolved function name and arguments can work but is quite ugly. I decided to handle the [] operator first.

checkAnswer(sql("select m['a'] from (select map('a', 'b') as m, 'aa' as aa)"), Row("b"))
checkErrorClass(
exception = intercept[AnalysisException] {
sql("select m[a] from (select map('a', 'b') as m, 'aa' as aa)")
Copy link
Contributor

Choose a reason for hiding this comment

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

In CheckAnalysis, how can we distinguish m.a and m[a]?

Copy link
Member Author

Choose a reason for hiding this comment

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

m.a is equivalent to m['a'] so it won't fail analysis

val orderedCandidates = StringUtils.orderStringsBySimilarity(unresolvedAttribute, candidates)
a.failAnalysis(
errorClass = errorClass,
messageParameters = Array(unresolvedAttribute, orderedCandidates.mkString(", ")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Both candidates and unresolvedAttribute should be wrapped with toSQLId

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. I will update this one after #36891 is merged.

Copy link
Contributor

@srielau srielau left a comment

Choose a reason for hiding this comment

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

Added a comment about toSQLId

@gengliangwang
Copy link
Member Author

Merging to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants