-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[MLv2] Prescribe the order for metabase.lib.column-group/group-columns
#38587
Conversation
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
|
{::lib.column-group/group-type :group-type/join.implicit | ||
:fk-join-alias "Orders" | ||
:fk-field-id (meta/id :orders :product-id)} |
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.
You missed this entry in the comment above
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.
Ah, it was included in one line, but that was left over from when the comment was phrased differently and grouped the joins rather than listing each one.
Fixed.
ede1e69
to
a480435
Compare
Adding |
3fe32ac
to
ee08be3
Compare
The desired order is: own columns, explicitly joined, implicitly joined. Explicit joins are ordered among themselves by join alias. Implicit joins are ordered by the join alias of the FK (own columns first), then by the name of the FK column. Previously this order has usually been at least close to the desired order by accident, since it's close to the iteration order of `visible-columns`. However, `group-columns` uses `group-by`, which returns a map. If there are enough groups (more than 8 I think) the map implementation will switch from a `[key1 value1 key2 value2 ...]` array to a real map, and the iteration order is undefined. This change enforces the desired order, including a test that `shuffle`s the columns. (And fixing the expectations for a few other tests.)
ee08be3
to
7690738
Compare
@bshepherdson Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
…ns` (#38587) The desired order is: own columns, explicitly joined, implicitly joined. Explicit joins are ordered among themselves by join alias. Implicit joins are ordered by the join alias of the FK (own columns first), then by the name of the FK column. Previously this order has usually been at least close to the desired order by accident, since it's close to the iteration order of `visible-columns`. However, `group-columns` uses `group-by`, which returns a map. If there are enough groups (more than 8 I think) the map implementation will switch from a `[key1 value1 key2 value2 ...]` array to a real map, and the iteration order is undefined. This change enforces the desired order, including a test that `shuffle`s the columns. (And fixing the expectations for a few other tests.)
…ns` (#38587) (#39036) The desired order is: own columns, explicitly joined, implicitly joined. Explicit joins are ordered among themselves by join alias. Implicit joins are ordered by the join alias of the FK (own columns first), then by the name of the FK column. Previously this order has usually been at least close to the desired order by accident, since it's close to the iteration order of `visible-columns`. However, `group-columns` uses `group-by`, which returns a map. If there are enough groups (more than 8 I think) the map implementation will switch from a `[key1 value1 key2 value2 ...]` array to a real map, and the iteration order is undefined. This change enforces the desired order, including a test that `shuffle`s the columns. (And fixing the expectations for a few other tests.) Co-authored-by: Braden Shepherdson <braden@metabase.com>
The desired order is: own columns, explicitly joined, implicitly joined.
Explicit joins are ordered among themselves by join alias. Implicit
joins are ordered by the join alias of the FK (own columns first), then
by the name of the FK column.
Previously this order has usually been at least close to the desired
order by accident, since it's close to the iteration order of
visible-columns
. However,group-columns
usesgroup-by
, whichreturns a map. If there are enough groups (more than 8 I think) the map
implementation will switch from a
[key1 value1 key2 value2 ...]
arrayto a real map, and the iteration order is undefined.
This change enforces the desired order, including a test that
shuffle
sthe columns. (And fixing the expectations for a few other tests.)
Fixes #38329.