-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Esql dimension aware attributes #131463
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
Esql dimension aware attributes #131463
Changes from 21 commits
05a4561
5f8e350
0c0b5bd
9d79b2f
2f9e7d2
c11fa12
28a3aea
001047d
3397960
038f743
d726c75
37849ed
cf48733
f246efa
3703533
47df3b4
5250dce
c7dfe13
ec5dfd4
2f638e7
1a16702
e7b7e7c
1b1afe5
89c2054
3b4dc17
9ab5c0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,4 +117,9 @@ protected NodeInfo<ReferenceAttribute> info() { | |
protected String label() { | ||
return "r"; | ||
} | ||
|
||
@Override | ||
public boolean isDimension() { | ||
return false; | ||
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. thought: it might get tricky when dealing with renames; if you have a 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. My understanding from @kkrik-es is that, at least for the initial version, that's an acceptable restriction. I think ReferenceAttributes are used for the output of EVAL and similar commands, and at least for the initial version of this we do not want to treat those as dimensions. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,6 +98,11 @@ protected String label() { | |
return UNRESOLVED_PREFIX; | ||
} | ||
|
||
@Override | ||
public boolean isDimension() { | ||
return false; | ||
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. thought: maybe this should throw UOE, because we can't determine this on an unresolved attr. |
||
} | ||
|
||
@Override | ||
public String nodeString() { | ||
return toString(); | ||
|
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.
I wonder if this should actually be ternary, resp. return
Boolean
(capitalB
) so that attributes can returnnull
to say "I don't know if I refer to a dimension" rather than claiming not to.Otherwise 👍
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.
What would be the behavior associated with the null version? The intention here is that we want to only allow filtering by dimensions, and enable automatic grouping by all dimensions. If a field returns null here, we would then need to decide if "null" behaved as "this is a dimension" or "this is not a dimension" for these cases.
Unless you're proposing that we fail (or null-with-warning) any query where there is ambiguity about the dimensions? I'm not sure I agree with that.