-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Multi-relation support, a pre-requisite for views and sub-queries #136780
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
Changes from 5 commits
ef5ab4e
9352610
81d7a17
b2f1fca
c56b904
922b96f
46cc712
a566cd2
629cc32
c884f12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3976,14 +3976,15 @@ required_capability: inline_stats | |
| required_capability: fix_join_output_merging | ||
|
|
||
| FROM languages_lookup_non_unique_key | ||
| | EVAL country = MV_SORT(country) | ||
| | KEEP country, language_name | ||
| | EVAL language_code = null::integer | ||
| | INLINE STATS MAX(language_code) BY language_code | ||
| | SORT country | ||
| | LIMIT 5 | ||
| ; | ||
|
|
||
| country:text |language_name:keyword |MAX(language_code):integer |language_code:integer | ||
| country:keyword |language_name:keyword |MAX(language_code):integer |language_code:integer | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am curious - why this change is happening in this pull?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could indeed be part of a separate PR, but the basic changes to CsvTestsDataLoader were made to facilitate views testing, and it was small enough that I thought it convenient to include in this PR. I already expect to have 3 or 4 PRs to complete the first functional Views, and hoped not to have even more. But if the changes to CsvTestsDataLoader and these two csv-spec files are cluttering this PR too much, I can move them out to another PR. |
||
| Atlantis |null |null |null | ||
| [Austria, Germany]|German |null |null | ||
| Canada |English |null |null | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -247,11 +247,12 @@ FROM employees | |
| | EVAL language_code = emp_no % 10 | ||
| | LOOKUP JOIN languages_lookup_non_unique_key ON language_code | ||
| | WHERE emp_no > 10090 AND emp_no < 10096 | ||
| | EVAL country = MV_SORT(country) | ||
| | SORT emp_no, country | ||
| | KEEP emp_no, language_code, language_name, country | ||
| ; | ||
|
|
||
| emp_no:integer | language_code:integer | language_name:keyword | country:text | ||
| emp_no:integer | language_code:integer | language_name:keyword | country:keyword | ||
| 10091 | 1 | English | Canada | ||
| 10091 | 1 | null | United Kingdom | ||
| 10091 | 1 | English | United States of America | ||
|
|
@@ -272,11 +273,12 @@ FROM employees | |
| | LIMIT 5 | ||
| | EVAL language_code = emp_no % 10 | ||
| | LOOKUP JOIN languages_lookup_non_unique_key ON language_code | ||
| | EVAL country = MV_SORT(country) | ||
| | KEEP emp_no, language_code, language_name, country | ||
| ; | ||
|
|
||
| ignoreOrder:true | ||
| emp_no:integer | language_code:integer | language_name:keyword | country:text | ||
| emp_no:integer | language_code:integer | language_name:keyword | country:keyword | ||
| 10001 | 1 | English | Canada | ||
| 10001 | 1 | English | null | ||
| 10001 | 1 | null | United Kingdom | ||
|
|
@@ -324,7 +326,7 @@ ROW language_code = 2 | |
|
|
||
| ignoreOrder:true | ||
| language_code:integer | country:text | language_name:keyword | ||
| 2 | [Austria, Germany] | German | ||
| 2 | [Germany, Austria] | German | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the order change?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is related to the mapping changes in CsvTestsDataLoader for the lookup index called languages_lookup_non_unique_key.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But as mentioned in my other comment, these changes could also be moved to a separate PR. I'd only included them because I will already have 3 or 4 PRs for Views, and hoped to not have even more than that. |
||
| 2 | Switzerland | German | ||
| 2 | null | German | ||
| ; | ||
|
|
||
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.
NIT: I think those 2 logs are redundant since we have per data set/enrich log entry.
Also I hope they are not loaded per test case as it would flood CI logs.
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.
They are logged only on the first test that needs data, then the EsqlSpecTestCase records that data is loaded and does not load it again. I found these very useful in debugging some odd cases where assertions killed cluster members. As mentioned in my responses to Stas before, these changes to the csv tests could actually be moved into a separate PR, but since I will already have 3 or 4 PRs for the Views MVP, I was hoping not to split the PRs up even further.