-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ES|QL] Reduce number of iterations on null columns #176174
Conversation
/ci |
/ci |
Seems fine but I want ci to run first |
/ci |
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
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.
Ci would complain if something went wrong but I also tested it locally in Chrome and everything works as expected. Thanx Marco for the follow up!
meta: { type: normalizeType(type) }, | ||
isNull: true, | ||
})) ?? []; | ||
allColumns.sort((a, b) => Number(a.isNull) - Number(b.isNull)); |
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.
Why do we sort? I think in UI it's ordered by name anyway.
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.
Because with ?drop_null_columns you get only the values from non empty columns.
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 but what would not work without sorting on allColumns
?
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 because you might have:
[{colA: null}, {colB: not null}, {colC: null}, {colD: not null}]
but on the values you get only
[1, 'a'] --> 1 is the value of col B and 'a' the value of colD
so if you don't put the null columns in the end then you are not having a correct match between your columns and values.
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.
Got it. Do we have a test to make sure that values are read correctly according to columns?
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 I added one in my PR (the one we merged)
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 👍
Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>
/ci |
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
## Summary Follow up on elastic#174585 Less code, less iterations. --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Stratoula Kalafateli <stratoula1@gmail.com> Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>
## Summary Follow up on elastic#174585 Less code, less iterations. --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Stratoula Kalafateli <stratoula1@gmail.com> Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>
## Summary Follow up on elastic#174585 Less code, less iterations. --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Stratoula Kalafateli <stratoula1@gmail.com> Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>
Summary
Follow up on #174585
Less code, less iterations.