You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by vrozov <gi...@git.apache.org> on 2017/08/30 00:09:33 UTC
[GitHub] drill pull request #926: DRILL-5269 Make DirectSubScan Jackson JSON deserial...
GitHub user vrozov opened a pull request:
https://github.com/apache/drill/pull/926
DRILL-5269 Make DirectSubScan Jackson JSON deserializable
@amansinha100 Please review
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vrozov/drill DRILL-5269
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/926.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 #926
----
commit 685653fed38beb4eea76cfe3460907386eb1a6c0
Author: Vlad Rozov <vr...@apache.org>
Date: 2017-08-30T00:05:24Z
DRILL-5269 Make DirectSubScan Jackson JSON deserializable
----
---
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 issue #926: DRILL-5269 Make DirectSubScan Jackson JSON deserializable
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/926
@paul-rogers Added unit test and adopted query to an existing parquet file.
---
[GitHub] drill pull request #926: DRILL-5269 Make DirectSubScan Jackson JSON deserial...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/926#discussion_r137658224
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/RecordReader.java ---
@@ -26,6 +26,9 @@
import org.apache.drill.exec.record.MaterializedField;
import org.apache.drill.exec.vector.ValueVector;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+
+@JsonTypeInfo(use=JsonTypeInfo.Id.NAME, include=JsonTypeInfo.As.WRAPPER_OBJECT, property="type")
--- End diff --
AFAIK, it will not impact implementations of the `RecordReader` interface. The annotation affects Jackson type inference when a physical operator has a reference to a `RecordReader` and Jackson needs to construct a concrete implementation of the `RecordReader`. Such information needs to be passed in any case and the annotation specifies JSON syntax used to pass the type information. To avoid the concern I'll move the annotation to the field declaration in `DirectSubScan`. The effect of such move is that it will be necessary to annotate `RecordReader` in every operator that may be passed as part of a fragment to a remote node for execution.
---
[GitHub] drill pull request #926: DRILL-5269 Make DirectSubScan Jackson JSON deserial...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/926#discussion_r137619550
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/RecordReader.java ---
@@ -26,6 +26,9 @@
import org.apache.drill.exec.record.MaterializedField;
import org.apache.drill.exec.vector.ValueVector;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+
+@JsonTypeInfo(use=JsonTypeInfo.Id.NAME, include=JsonTypeInfo.As.WRAPPER_OBJECT, property="type")
--- End diff --
I am not a Jackson expert; since this annotation is in the core RecordReader interface, what impact does it have on all the implementers of this interface ?
---
[GitHub] drill issue #926: DRILL-5269 Make DirectSubScan Jackson JSON deserializable
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/926
On the first comment, it sounds like the close is a placeholder, actual close is to be done after some refactoring? This is fine. Just a bit surprising to see a log message, but not actual close.
On the second, yes, Drill uses "unit test" to mean a system test run via JUnit. Can we create a JUnit-based system test that demonstrates the problem and fix? I'm guessing that we can do the test at the system level to avoid the difficulty, at present, of writing true unit tests against Drill code.
---
[GitHub] drill issue #926: DRILL-5269 Make DirectSubScan Jackson JSON deserializable
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/926
@paul-rogers I guess you refer to a system/integration test (execute the query provided in JIRA), not a unit test.
---
[GitHub] drill issue #926: DRILL-5269 Make DirectSubScan Jackson JSON deserializable
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/926
rebased and squashed
---
[GitHub] drill pull request #926: DRILL-5269 Make DirectSubScan Jackson JSON deserial...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/926
---