Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adds parsing and modeling for
INSERT INTO <table name>
#1621base: prep-v0_14_10
Are you sure you want to change the base?
Adds parsing and modeling for
INSERT INTO <table name>
#1621Changes from 1 commit
66af4ba
005ed72
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
A change I've been wanting to make forever is to make
uniqueId
here anIonElement
so we can put arbitrary Ion data here instead of having to "abuse" this field by putting Ion-text here instead. I'll leave a note in the main PIG domain where we need to change this as well.... EDIT: github won't let me comment there--to do this we would also need to changepartiql_logical_resolved.expr.global_id.unique_id
to theion
type.Since we're breaking this API anyway, now might be a good time to do that as well...
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.
IMHO this might be a little too forward thinking... i.e. trying to design for a future we know little about. Or is there more to this than I am aware of?
In my case, all parts of a table identifier will be completely resolved or we will abort w/undefined table error. This might make sense in the event that we have only part of the schema... but that is not in my foreseeable future. I suggest dropping this.
If this is dropped, I don't think we need a distinction between a
GlobalVariable
and aNamespacedVariable
either...GlobalVariable
will still be fine for our case... Even though we're kind of using it in a way not entirely consistent with the name. Maybe, "ResolvedVariable" is a better 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.
Thinking again about this... was the intention to handle the scenario when some of the parts of the qualified identifier could be resolved but others not? I guess I'm not 100% sure I understand the intent here.
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.
Yes, that is the intention. In the
INSERT INTO <table name>
use-case, you wouldn't allowremainingSteps
to have anything. But, in a query such as the one mentioned in the Javadocs, it is useful to know what is actually a global vs what is a path expression. This allows for the engine-implementer to enforce the accuracy of the path expression and for the global-variable-resolver implementer to only take care of returning "global" variables.This is something that has been added to
v1
.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 think you can drop this--it is redundant when you have
resolveGlobal(BindingId)
. Or, more accurately, drop this function and move its documentation (with modification) to the other overload. Then, remove the default implementation ofresolveGlobal(BindingId)
so we're forced to implement it.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.
For backwards compatibility purposes, when adding new APIs, we've been adding default implementations that "may" utilize existing implemented methods, so that users who don't want to update their code don't have to. That being said, you're the only customer here, so it's up to you.