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
SQL: Preserve original source for each expression #36912
Conversation
allowing accurate reproducibility of the source Fix elastic#36894
Pinging @elastic/es-search |
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.
LGTM.
Left few comments regarding the string manipulating functions that have one of their parameters called source
: REPLACE(), INSERT(), LOCATE() and SUBSTRING(). I was suggesting in my comments to have the source
parameter name changed to something like sourceText
and not changing the return method name to sourcePipe()
.
@@ -90,7 +90,7 @@ public InsertFunctionProcessor asProcessor() { | |||
return new InsertFunctionProcessor(source.asProcessor(), start.asProcessor(), length.asProcessor(), replacement.asProcessor()); | |||
} | |||
|
|||
public Pipe source() { | |||
public Pipe sourcePipe() { |
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.
Maybe change the name of the rest of the methods to the *Pipe()
format? It doesn't look consistent... source
has its method called sourcePipe()
while start
and the rest have their methods called start()
, length()
, replacement()
. Or maybe change the name of the source
field to sourceText
and have its return method called sourceText()
. I would be in favor of the second option, to remain consistent with the rest of the functions (that don't have Pipe
in their return methods).
@@ -82,7 +82,7 @@ public LocateFunctionProcessor asProcessor() { | |||
return new LocateFunctionProcessor(pattern.asProcessor(), source.asProcessor(), start == null ? null : start.asProcessor()); | |||
} | |||
|
|||
public Pipe source() { | |||
public Pipe sourcePipe() { |
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.
Same comment here as the one for Insert
function: maybe change source
to sourceText
and don't use Pipe
in the return method name.
@@ -78,7 +78,7 @@ public ReplaceFunctionProcessor asProcessor() { | |||
return new ReplaceFunctionProcessor(source.asProcessor(), pattern.asProcessor(), replacement.asProcessor()); | |||
} | |||
|
|||
public Pipe source() { | |||
public Pipe sourcePipe() { |
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.
Here, as well.
@@ -78,7 +78,7 @@ public SubstringFunctionProcessor asProcessor() { | |||
return new SubstringFunctionProcessor(source.asProcessor(), start.asProcessor(), length.asProcessor()); | |||
} | |||
|
|||
public Pipe source() { | |||
public Pipe sourcePipe() { |
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.
Here
@@ -95,13 +94,13 @@ public void testSortFieldSpecified() { | |||
FieldSortBuilder sortField = fieldSort("test").unmappedType("keyword"); | |||
|
|||
QueryContainer container = new QueryContainer() | |||
.sort(new AttributeSort(new FieldAttribute(new Location(1, 1), "test", new KeywordEsField("test")), Direction.ASC, | |||
.sort(new AttributeSort(new FieldAttribute(Source.EMPTY, "test", new KeywordEsField("test")), Direction.ASC, |
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.
Here you replaced new Location(1,1)
with Source.EMPTY
, but here you had a different approach.
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.
LGTM. I agree with Andrei's comments about the sourcePipe
.
On top I'd suggest to change the methods' parameter name from location
to source
, e.g.: https://github.com/elastic/elasticsearch/pull/36912/files#diff-f90669a222551c32dc8c7fdf880eba57R68 This will create a few conflicts as some expression params are names source
which can be renamed to something else like sourceStr
or sourceText
as Andrei said.
import static org.hamcrest.Matchers.equalTo; | ||
|
||
public class MatchQueryTests extends ESTestCase { | ||
static MatchQuery randomMatchQuery() { | ||
return new MatchQuery( | ||
LocationTests.randomLocation(), | ||
SourceTests.randomSource(), |
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.
Please fix identation.
run the gradle build tests 1 |
The source text is saved alongside the location of each Node/Expression allowing accurate reproducibility of the source
Fix #36894
The PR is small but it is wide spread hence the huge number of files involved.
I'd like to add more tests to check the source is being properly preserved.
A follow-up will make sure the source is displayed as is for naming.