-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[CALCITE-7780] Avoid redundant map lookups #5262
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
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 |
|---|---|---|
|
|
@@ -61,16 +61,16 @@ public UncollectNode(Compiler compiler, Uncollect uncollect) { | |
| } | ||
| } | ||
| } else if (value instanceof Map) { | ||
| Map map = (Map) value; | ||
| Map<?, ?> map = (Map<?, ?>) value; | ||
| if (map.isEmpty() && rel.isOuter) { | ||
| sink.send(Row.of(new Object[width])); | ||
| continue; | ||
| } | ||
| for (Object key : map.keySet()) { | ||
| for (Map.Entry<?, ?> entry : map.entrySet()) { | ||
|
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.
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. The int i is being modified within this loop, so that would need to be wrapped in order to modify it, if we went that route. entrySet seems like a better fit here |
||
| if (rel.withOrdinality) { | ||
| sink.send(Row.of(key, map.get(key), i++)); | ||
| sink.send(Row.of(entry.getKey(), entry.getValue(), i++)); | ||
| } else { | ||
| sink.send(Row.of(key, map.get(key))); | ||
| sink.send(Row.of(entry.getKey(), entry.getValue())); | ||
| } | ||
| } | ||
| } else { | ||
|
|
||
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.
Some Java maps allow null keys. Have you checked that this one doesn't?
If it does, the semantics is not the same.
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.
In this case the values will always be non-null. Should I comment in these cases to make it more clear this is safe?
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.
Fortunately we have
@Nullableannotations, so if the ksy is not nullable, hopefully the invariant is maintained.