You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by chunhui-shi <gi...@git.apache.org> on 2016/12/21 20:49:59 UTC
[GitHub] drill pull request #702: DRILL-5088: set default codec for toJson
GitHub user chunhui-shi opened a pull request:
https://github.com/apache/drill/pull/702
DRILL-5088: set default codec for toJson
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chunhui-shi/drill DRILL-5088
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/702.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 #702
----
commit c285a334eaeda810150ff162d1e1c0da342a37ff
Author: chunhui-shi <cs...@maprtech.com>
Date: 2016-12-18T08:27:50Z
DRILL-5088: set default codec for toJson
----
---
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 #702: DRILL-5088: set default codec for toJson
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on the issue:
https://github.com/apache/drill/pull/702
+1 LGTM
---
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 #702: DRILL-5088: set default codec for toJson
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/702
---
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 #702: DRILL-5088: set default codec for toJson
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/702#discussion_r95719700
--- Diff: contrib/storage-mongo/src/main/java/org/apache/drill/exec/store/mongo/MongoGroupScan.java ---
@@ -503,7 +505,9 @@ public ScanStats getScanStats() {
long numDocs = collection.count();
float approxDiskCost = 0;
if (numDocs != 0) {
- String json = collection.find().first().toJson();
+ final DocumentCodec codec =
--- End diff --
Please add a comment to describe the need for the codec along with the example from the bug.
---
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 #702: DRILL-5088: set default codec for toJson
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the issue:
https://github.com/apache/drill/pull/702
+1
---
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 #702: DRILL-5088: set default codec for toJson
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on the issue:
https://github.com/apache/drill/pull/702
MongoGroupScan tries to read first record to guess record width. So this toJson is called for only first record.
Notice that MongoClient.getDefaultCodecRegistry() contains DBRefCodecProvider:
Shown in MongoClient.java(In MongoClient.java):
private static final CodecRegistry DEFAULT_CODEC_REGISTRY =
fromProviders(asList(new ValueCodecProvider(),
new DBRefCodecProvider(),
new DocumentCodecProvider(new DocumentToDBRefTransformer()),
new DBObjectCodecProvider(),
new BsonValueCodecProvider(),
new GeoJsonCodecProvider()));
but a default DocumentCodec constructor does not. And it was this constructor get called if we don't set a codec for toJson function to use. The list of CodecProviders is shown in DocumentCodec.java in MongoDb's code:
public class DocumentCodec implements CollectibleCodec<Document> {
private static final String ID_FIELD_NAME = "_id";
private static final CodecRegistry DEFAULT_REGISTRY = fromProviders(asList(new ValueCodecProvider(),
new BsonValueCodecProvider(),
new DocumentCodecProvider()));
---
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 #702: DRILL-5088: set default codec for toJson
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/702#discussion_r99396637
--- Diff: contrib/storage-mongo/src/test/java/org/apache/drill/exec/store/mongo/MongoTestSuit.java ---
@@ -234,15 +240,17 @@ private static void createDbAndCollections(String dbName,
@AfterClass
public static void tearDownCluster() throws Exception {
- if (mongoClient != null) {
- mongoClient.dropDatabase(EMPLOYEE_DB);
- mongoClient.close();
- }
synchronized (MongoTestSuit.class) {
- if (distMode) {
- DistributedMode.cleanup();
- } else {
- SingleMode.cleanup();
+ if (initCount.decrementAndGet() == 0) {
+ if (mongoClient != null) {
+ mongoClient.dropDatabase(EMPLOYEE_DB);
+ mongoClient.close();
+ }
+ if (distMode) {
+ DistributedMode.cleanup();
+ } else {
+ SingleMode.cleanup();
--- End diff --
Should the above include error handling? It seems that if the `dropDatabase` fails, then `close()` won't be called, nor will the `cleanup` method.
---
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 #702: DRILL-5088: set default codec for toJson
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on the issue:
https://github.com/apache/drill/pull/702
Thanks for adding the testcase.
+1 LGTM.
---
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 #702: DRILL-5088: set default codec for toJson
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on a diff in the pull request:
https://github.com/apache/drill/pull/702#discussion_r99448154
--- Diff: contrib/storage-mongo/src/test/java/org/apache/drill/exec/store/mongo/TestTableGenerator.java ---
@@ -58,7 +59,16 @@ public static void generateTable(String dbName, String collection,
.jsonArray(jsonArray).importFile(jsonFile).build();
MongoImportExecutable importExecutable = MongoImportStarter
.getDefaultInstance().prepare(mongoImportConfig);
- importExecutable.start();
+ MongoImportProcess importProcess = importExecutable.start();
+
+ try {
+ while (importProcess.isProcessRunning()) {
+ Thread.sleep(1000);
+ }
+ }catch (Exception ex) {
+ logger.error("Import mongoDb failed", ex);
--- End diff --
Paul, since there are two JIRAs here, I filed a new pull request https://github.com/apache/drill/pull/741 which addressed your comments. Could you look at that pull request? 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 #702: DRILL-5088: set default codec for toJson
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/702#discussion_r99396951
--- Diff: contrib/storage-mongo/src/test/java/org/apache/drill/exec/store/mongo/TestTableGenerator.java ---
@@ -58,7 +59,16 @@ public static void generateTable(String dbName, String collection,
.jsonArray(jsonArray).importFile(jsonFile).build();
MongoImportExecutable importExecutable = MongoImportStarter
.getDefaultInstance().prepare(mongoImportConfig);
- importExecutable.start();
+ MongoImportProcess importProcess = importExecutable.start();
+
+ try {
+ while (importProcess.isProcessRunning()) {
+ Thread.sleep(1000);
+ }
+ }catch (Exception ex) {
+ logger.error("Import mongoDb failed", ex);
--- End diff --
Here we log the error but go ahead and return. Should we propagate an exception upward in the call stack so that the caller knows that "Something Is Wrong"? Otherwise, how will the caller know whether the import process is ready or not?
---
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.
---