Skip to content

Conversation

@krisds
Copy link
Contributor

@krisds krisds commented Feb 1, 2019

As requested, I will split #38033 into separate PRs. Here is the third part.

This one resolves:

@jimczi jimczi added the :Analytics/SQL SQL querying label Feb 1, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@costin
Copy link
Member

costin commented Feb 1, 2019

Thanks for raising the PR.
However I don't think the proposed solution is correct; I can see how an analysis tool will pick it up (Literal and NamedExpression used to be different hierarchies which got unified at some point though they might be split again).
As the literal already inherits the NamedExpression behavior , the correct solution would be to remove the literal check all together - let me know if you want to pursue this or not.
Thanks,

@krisds
Copy link
Contributor Author

krisds commented Feb 1, 2019

Thanks. I was unsure what the right solution was. If you like I can remove the literal check and update the PR.

@costin
Copy link
Member

costin commented Feb 2, 2019

Looks like I've missed the PRs and added here the comment that was made for
#38150.

This is another instance of code left behind from a structural refactoring (making functions named expressions). I'm on the fence on removing this since the code has meaning, especially as functions will be made again non-named expressions in which case the order is important...

@krisds
Copy link
Contributor Author

krisds commented Feb 5, 2019

So, should I remove the check, or will you leave it as is and close the PR ?

@javanna
Copy link
Member

javanna commented Feb 25, 2019

@costin could you please merge this or close it if you think the code is fine as-is? Thanks!

@costin
Copy link
Member

costin commented Feb 25, 2019

Apologies for the late reply - this got lost in my inbox.
I'm opting for leaving this as is since it plays into an upcoming refactoring.
Thanks for raising the PR @krisds.

Cheers,

@costin costin closed this Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants