You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by hsuanyi <gi...@git.apache.org> on 2015/11/03 18:08:36 UTC
[GitHub] drill pull request: DRILL-4006: Ensure the position of MapWriters ...
GitHub user hsuanyi opened a pull request:
https://github.com/apache/drill/pull/233
DRILL-4006: Ensure the position of MapWriters is set
This can help eliminate potential IOOB exception
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hsuanyi/incubator-drill DRILL-4006
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/233.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #233
----
commit 4c0048090f56c6e017ffbad20170af46cbf53b41
Author: Hsuan-Yi Chu <hs...@usc.edu>
Date: 2015-11-01T00:14:34Z
DRILL-4006: Ensure the position of MapWriters is set
This can help eliminate potential IOOB exception
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4006: Ensure the position of MapWriters ...
Posted by zfong <gi...@git.apache.org>.
Github user zfong commented on the pull request:
https://github.com/apache/drill/pull/233#issuecomment-153421208
Is it possible to add a unit test for this problem? Seems like a pretty straightforward repro. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4006: Ensure the position of MapWriters ...
Posted by hsuanyi <gi...@git.apache.org>.
Github user hsuanyi commented on the pull request:
https://github.com/apache/drill/pull/233#issuecomment-153421763
Will do after I rebase it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4006: Ensure the position of MapWriters ...
Posted by hsuanyi <gi...@git.apache.org>.
Github user hsuanyi closed the pull request at:
https://github.com/apache/drill/pull/233
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4006: Ensure the position of MapWriters ...
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/233#discussion_r43777175
--- Diff: exec/java-exec/src/main/codegen/templates/MapWriters.java ---
@@ -110,9 +110,9 @@ public ListWriter list(String name) {
FieldWriter writer = fields.get(name.toLowerCase());
if(writer == null) {
writer = new SingleListWriter(name, container, this);
- writer.setPosition(${index});
fields.put(name.toLowerCase(), writer);
}
+ writer.setPosition(${index});
--- End diff --
Your change seems to violate the design pattern of the writers. Position is managed via the setPosition call at line 152. If you're seeing problems, you should figure who isn't calling that method. I believe the code before your change is correct: the only reason we should have to reposition a writer when we retrieve it is it didn't previously exist and we had to create a new one. This is the pattern used across the board for position in the context of all the writers. Position is managed externally and only a new field creation should cause a location position set.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---