-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Rework QualifiedName. #1903
Rework QualifiedName. #1903
Conversation
if len(n) > 1 { | ||
return n[0] | ||
func (n *QualifiedName) Database() string { | ||
if len(n.Indirect) > 0 { |
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.
can this just always return n.Base.String()
?
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.
No. While that would work with the current tests, I think it is only due to the way we always normalize the qualified name before using the Database()
method. This logic here is that an expression like foo
(with no indirection expression), foo
represents a table or column name and not the database name. I'll add a comment to explain this.
4e37906
to
2e9d701
Compare
} | ||
return "" | ||
} | ||
|
||
// Table returns the table portion of the name. | ||
func (n QualifiedName) Table() string { | ||
return n[len(n)-1] | ||
func (n *QualifiedName) Table() string { |
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 function's correctness still depends on QualifiedName not referring to a column, is that right? maybe it deserves a comment.
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, sort of. The use of QualifiedName
is context sensitive. In some contexts it refers to a table and in others to a column. I'll add a node to see the comment about Column
.
LGTM |
2e9d701
to
efa9a37
Compare
} | ||
fmt.Fprintf(&buf, "*") | ||
return buf.String() | ||
return "*" |
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 thought QualifiedName
took care of that now?
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 have it on my own personal TODO list to figure out if this can be further simplified. A QualifiedName
can contain a *
, but not as the only element or the first element which is a Name
.
QualifiedName is now composed of a base name and an indirection expression. Elements in an indirection expression are either ".<name>", ".*" or "[<begin>:<end>]". Fixes #1810.
efa9a37
to
b7b792b
Compare
QualifiedName is now composed of a base name and an indirection
expression. Elements in an indirection expression are either ".",
".*" or "[:]".
Fixes #1810.