[GitHub] drill pull request: Bugs/various work re master 07 09

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/87

Bugs/various work re master 07 09



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill 
bugs/various_WORK_re_master_07-09

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/87.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 #87


commit 92e2cc18c79dbf5269304d1479f1b6ff8bcd477e
Author: dbarclay 
Date:   2015-03-23T21:16:48Z

sep.:  --- forkCount 2 -> 1 [pom.xml] ---  >= 
Working, < Review

commit 59608f3664cefd9754b778c45479c3b72025b1b2
Author: dbarclay 
Date:   2015-03-24T17:20:27Z

sep.:  =-=-=-=-=-=-=-= forkCount 3 -> 2.  [/pom.xml] =-=-=-=-=-=-=-=  >= in 
Review, < awaiting Merge

Conflicts:
pom.xml

commit 3e155dc1345118a997de66b338adead8328517bd
Author: dbarclay 
Date:   2015-06-20T02:05:39Z

DRILL-3151:  Fix many ResultSetMetaData method return values.

Added ~unit test for ResultSetMetaData implementation.

Made getObject return classes available to implementation of 
getColumnClassName:
- Added SqlAccessor.getObjectClass() (to put that metadata right next to 
code
  to which it corresponds rather than in far-away parallel code).
- Added similar AvaticaDrillSqlAccessor.getObjectClass().
- Changed DrillAccessorList.accessors from Accessor[] to
  AvaticaDrillSqlAccessor[] for better access to JDBC getObject return 
class.
- Extracted return classes from accessors to pass to updateColumnMetaData.

Reworked some data type mapping and utilities:
- Added Added Types.getSqlTypeName(...).
- Renamed Types.getJdbcType(...) to getJdbcTypeCode(...)
- Replaced Types.isUnSigned with isJdbcSignedType.
- Fixed various bogus RPC-type XXX -> java.sql.Types.SMALLINT mappings.
- Removed DrillColumnMetaDataList.getJdbcTypeName.
- Moved getAvaticaType up (for bottom-up order).
- Revised DrillColumnMetaDataList.getAvaticaType(...).

MAIN:
- Updated updateColumnMetaData(...) to change many calculations of metadata
  input to ColumnMetaData construction.  [DrillColumnMetaDataList]

Updated other metadata tests per changes.

commit ceae4668f250672391e51c84d9b4e295a4c0f4a5
Author: dbarclay 
Date:   2015-07-02T22:04:31Z

DRILL-3483: Clarify CommonConstants' constants.  [CommonConstants, 
DrillConfig, PathScanner]

Renamed constants.
Documented constants.
Removed extraneous "public static final" (redudant and abnormal since in 
interface).

commit 03dafaca207b45c01519e22ceeb0dd2784db18d5
Author: dbarclay 
Date:   2015-04-17T20:09:59Z

DRILL-2696: Test for future DRILL-2696 fix (currently disabled with 
@Ignore).

commit 73183a415ac765863ade2a632799f328fcddad74
Author: dbarclay 
Date:   2015-04-17T23:27:46Z

DRILL-2815: Some PathScanner logging, misc. cleanup.

Add some DEBUG-level log calls and augmented, edited log message.

Misc. code "hygiene":
- Added method doc comment.
- Renamed to clarify a number of names.
- Added "final".
- Fixed indentation; wrapped some long lines.

commit c68cc505c03b06b2db3d18eef7bce965408efb15
Author: dbarclay 
Date:   2015-03-29T21:46:47Z

temp:  logging adjustments.  [exec/java-exec/src/test/resources/logback.xml]

commit e48f5267c6c1b27cd2fa500a8062876a61e0f6d2
Author: dbarclay 
Date:   2015-03-29T21:47:11Z

temp:  logging adjustments.  [exec/jdbc/src/test/resources/logback.xml]

commit a0e9cb671e08ded93e571fa096fa5c38f765ed85
Author: dbarclay 
Date:   2015-04-06T18:11:13Z

temp:  logging adjustments.  [common/src/test/resources/logback.xml]

commit 321b9d09da0973e8da48f81a94c64b3503602321
Author: dbarclay 
Date:   2015-05-10T19:33:21Z

temp:  logging adjustments.  [distribution/src/resources/logback.xml]

commit d7da49862c13144c29d78d49c9d4605952f436cf
Author: dbarclay 
Date:   2015-05-14T22:40:06Z

temp:  logging adjustments (probably TEMP).  
[exec/jdbc/src/test/resources/logback.xml]

commit ae797565156fbaefa795ca22d75631763bd4cfe6
Author: dbarclay 
Date:   2015-05-15T03:54:51Z

temp:  logging adjustments.  [jdbc/src/test/resources/logback.xml]

commit 1cd642ad80311850c4df1f76099a05c78b8a6a31
Author: dbarclay 
Date:   2015-04-17T20:54:22Z

:  Logging:  Added calls.  [DrillConfig]

commit 11a72689afc4e3c86cc1dcf6fbd5213040cf444e
Author: dbarclay 
Date:   2015-05-30T18:11:13Z

temp:  logging adjustments.  [jdbc/src/test/resources/logback.xml]

commit d7554c584242434d834640dd746c5fc7bfb4c812
Author: dbarclay 
Date:   2015-05-31T00:45:12Z

temp:  logging adjustments.  [java-exec/src/test/resources/logback.xml]

commit 6179813f4c7c149f1d9055842f14d097c3c24bc2

[GitHub] drill pull request: Bugs/various work re master 07 09

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/87


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3483: Clarify CommonConstants' constants...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/88

DRILL-3483: Clarify CommonConstants' constants.

Renamed constants.
Documented constants.
Removed extraneous "public static final" (redundant and abnormal since in 
interface).

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill 
bugs/drill-3483-CommonConstants-clarification

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/88.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 #88


commit 6e39eb7ca428d0d0baeeb29f50ed25b1a8872685
Author: dbarclay 
Date:   2015-07-02T22:04:31Z

DRILL-3483: Clarify CommonConstants' constants.  [CommonConstants, 
DrillConfig, PathScanner]

Renamed constants.
Documented constants.
Removed extraneous "public static final" (redudant and abnormal since in 
interface).




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3483: Clarify CommonConstants' constants...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/88#issuecomment-121101705
  
Ping?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: Enhanced logging and associated code hygiene

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/93

Enhanced logging and associated code hygiene



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill 
bugs/drill-2815-etc-Logging

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/93.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 #93


commit 24390a34e6886d019382f0e0c00cab570a499045
Author: dbarclay 
Date:   2015-07-14T04:02:40Z

DRILL-2815: Augment PathScanner logging; clean up code.

Added some DEBUG- and TRACE-level log calls.
Augmented/edited existing log call.

Misc. code hygiene:
- Added documentation of forResources(...).
- Renamed to clarify a number of local names.
- Made logger private.
- Added "final".
- Fixed indentation; wrapped some long lines; etc.

Conflicts:
common/src/main/java/org/apache/drill/common/util/PathScanner.java

commit e740ac4f7acc022922b5e5eebe209de42c504406
Author: dbarclay 
Date:   2015-07-14T19:11:36Z

DRILL-3496: Augment logging in DrillConfig and classpath scanning.

Main: Added and augmented logging calls.

Code Hygiene:
- Added/revised doc. comments, code comments.
- Renamed some locals.
- Added "final".
- Wrapped lines.

Also fixed currentTimeMillis -> Stopwatch in Drillbit.

Conflicts:
common/src/main/java/org/apache/drill/common/config/DrillConfig.java




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: Enhanced logging and associated code hygiene

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r34642779
  
--- Diff: 
common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -44,26 +46,33 @@
 import com.google.common.collect.ImmutableList;
 import com.typesafe.config.Config;
 import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigRenderOptions;
 
 public final class DrillConfig extends NestedConfig{
-//  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
+  private static final Logger logger = getLogger(DrillConfig.class);
--- End diff --

To get rid of the unnecessarily verbose qualified names in that boilerplate 
code (which, when having any unnecessary verbosity, adds unnecessary risk of 
not noticing mistakes like this:

public class TestMultiInputAdd extends PopUnitTestBase {

//private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(TestMathFunctions.class);

)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: Enhanced logging and associated code hygiene

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r34649690
  
--- Diff: 
common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +155,77 @@ public static DrillConfig create(Properties 
testConfigurations) {
 return create(null, testConfigurations, true);
   }
 
-  public static DrillConfig create(String overrideFileName, boolean 
enableServerConfigs) {
-return create(overrideFileName, null, enableServerConfigs);
+  /**
+   * ...
+   * @param overrideFileResourcePathname
+   *  see {@link #create(String)}'s {@code 
overrideFileResourcePathname}
+   */
+  public static DrillConfig create(String overrideFileResourcePathname, 
boolean enableServerConfigs) {
+return create(overrideFileResourcePathname, null, enableServerConfigs);
   }
 
-  private static DrillConfig create(String overrideFileName, Properties 
overriderProps, boolean enableServerConfigs) {
-overrideFileName = overrideFileName == null ? 
CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
-// first we load defaults.
+  /**
+   * ...
+   * @param overrideFileResourcePathname
+   *  see {@link #create(String)}'s {@code 
overrideFileResourcePathname}
+   * @param overriderProps
+   *  optional property map for further overriding (after override 
file
+   *  is assimilated
+   * @param enableServerConfigs
+   *  whether to enable server-specific configuration options
+   * @return
+   */
+  private static DrillConfig create(String overrideFileResourcePathname,
+final Properties overriderProps,
+final boolean enableServerConfigs) {
+overrideFileResourcePathname =
+overrideFileResourcePathname == null
+? CommonConstants.CONFIG_OVERRIDE
+: overrideFileResourcePathname;
+
+// 1. Load defaults configuration file.
 Config fallback = null;
 final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
 for (ClassLoader classLoader : classLoaders) {
-  if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) 
{
-fallback = ConfigFactory.load(classLoader, 
CommonConstants.CONFIG_DEFAULT);
+  final URL url =
+  classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+  if (null != url) {
+logger.debug("Loading base config. file at {}.", url);
+fallback =
+ConfigFactory.load(classLoader,
+   CommonConstants.CONFIG_DEFAULT);
 break;
   }
 }
 
+// 2. Load per-module configuration files.
 Collection urls = PathScanner.getConfigURLs();
 logger.debug("Loading configs at the following URLs {}", urls);
 for (URL url : urls) {
+  logger.debug("Loading module config. file at {}.", url);
--- End diff --

First call removed in update.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: Enhanced logging and associated code hygiene

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r34650500
  
--- Diff: 
common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +155,77 @@ public static DrillConfig create(Properties 
testConfigurations) {
 return create(null, testConfigurations, true);
   }
 
-  public static DrillConfig create(String overrideFileName, boolean 
enableServerConfigs) {
-return create(overrideFileName, null, enableServerConfigs);
+  /**
+   * ...
+   * @param overrideFileResourcePathname
+   *  see {@link #create(String)}'s {@code 
overrideFileResourcePathname}
+   */
+  public static DrillConfig create(String overrideFileResourcePathname, 
boolean enableServerConfigs) {
+return create(overrideFileResourcePathname, null, enableServerConfigs);
   }
 
-  private static DrillConfig create(String overrideFileName, Properties 
overriderProps, boolean enableServerConfigs) {
-overrideFileName = overrideFileName == null ? 
CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
-// first we load defaults.
+  /**
+   * ...
+   * @param overrideFileResourcePathname
+   *  see {@link #create(String)}'s {@code 
overrideFileResourcePathname}
+   * @param overriderProps
+   *  optional property map for further overriding (after override 
file
+   *  is assimilated
+   * @param enableServerConfigs
+   *  whether to enable server-specific configuration options
+   * @return
+   */
+  private static DrillConfig create(String overrideFileResourcePathname,
+final Properties overriderProps,
+final boolean enableServerConfigs) {
+overrideFileResourcePathname =
+overrideFileResourcePathname == null
+? CommonConstants.CONFIG_OVERRIDE
+: overrideFileResourcePathname;
+
+// 1. Load defaults configuration file.
 Config fallback = null;
 final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
 for (ClassLoader classLoader : classLoaders) {
-  if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) 
{
-fallback = ConfigFactory.load(classLoader, 
CommonConstants.CONFIG_DEFAULT);
+  final URL url =
+  classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+  if (null != url) {
+logger.debug("Loading base config. file at {}.", url);
+fallback =
+ConfigFactory.load(classLoader,
+   CommonConstants.CONFIG_DEFAULT);
 break;
   }
 }
 
+// 2. Load per-module configuration files.
 Collection urls = PathScanner.getConfigURLs();
 logger.debug("Loading configs at the following URLs {}", urls);
 for (URL url : urls) {
+  logger.debug("Loading module config. file at {}.", url);
   fallback = ConfigFactory.parseURL(url).withFallback(fallback);
 }
 
-Config effectiveConfig = 
ConfigFactory.load(overrideFileName).withFallback(fallback);
+// 3. Load any overrides configuration file and overrides from JVM
--- End diff --

That "overrides" is part of the noun phrase "overrides configuration file" 
(parallel to "defaults configuration file" and "per-module configuration file") 
(not a reference to the overrides from any such file).

Yes, it's a bit ambiguous between meaning that and seeming like a word is 
missing if one doesn't see the other phrases. (Changing the "any" to "the" 
might reduce that ambiguity, but then it would imply that that code expects 
there to be an overrides configuration file, when it's actually optional.)





---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: Enhanced logging and associated code hygiene

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/93#issuecomment-121513460
  
> ... clutter Lilith ... 3) Every other log statement in your patch should 
be TRACE level. ...

Commit 872309a... reverts to one log call and log entry per list, but 
retains the improved readability by putting each listed item on a separate line.

> ... 1) Overrides in DrillConfig (step 3 and 4 in DrillConfig#create) 
could be at INFO level. ...

Since the set of per-module configuration files is affected by user action 
(installing plug-ins), step 2's logging should probably be at the same level. 

And then, since that's reporting every configuration file except the 
defaults file, maybe the defaults file should also be listed at the same level.

Comments?  (INFO for all those config files?)




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r35052392
  
--- Diff: 
common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -90,7 +97,8 @@ public DrillConfig(Config config, boolean enableServer) {
   }
 
   /**
-   * Create a DrillConfig object using the default config file name
+   * Creates a DrillConfig object using the default config file name
+   * and with server-specific configuration options disabled.
--- End diff --

Fixed.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r35052980
  
--- Diff: 
common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +155,77 @@ public static DrillConfig create(Properties 
testConfigurations) {
 return create(null, testConfigurations, true);
   }
 
-  public static DrillConfig create(String overrideFileName, boolean 
enableServerConfigs) {
-return create(overrideFileName, null, enableServerConfigs);
+  /**
+   * ...
+   * @param overrideFileResourcePathname
+   *  see {@link #create(String)}'s {@code 
overrideFileResourcePathname}
+   */
+  public static DrillConfig create(String overrideFileResourcePathname, 
boolean enableServerConfigs) {
+return create(overrideFileResourcePathname, null, enableServerConfigs);
   }
 
-  private static DrillConfig create(String overrideFileName, Properties 
overriderProps, boolean enableServerConfigs) {
-overrideFileName = overrideFileName == null ? 
CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
-// first we load defaults.
+  /**
+   * ...
+   * @param overrideFileResourcePathname
+   *  see {@link #create(String)}'s {@code 
overrideFileResourcePathname}
+   * @param overriderProps
+   *  optional property map for further overriding (after override 
file
+   *  is assimilated
+   * @param enableServerConfigs
+   *  whether to enable server-specific configuration options
+   * @return
+   */
+  private static DrillConfig create(String overrideFileResourcePathname,
+final Properties overriderProps,
+final boolean enableServerConfigs) {
+overrideFileResourcePathname =
+overrideFileResourcePathname == null
+? CommonConstants.CONFIG_OVERRIDE
+: overrideFileResourcePathname;
+
+// 1. Load defaults configuration file.
 Config fallback = null;
 final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
 for (ClassLoader classLoader : classLoaders) {
-  if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) 
{
-fallback = ConfigFactory.load(classLoader, 
CommonConstants.CONFIG_DEFAULT);
+  final URL url =
+  classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+  if (null != url) {
+logger.debug("Loading base config. file at {}.", url);
+fallback =
+ConfigFactory.load(classLoader,
+   CommonConstants.CONFIG_DEFAULT);
 break;
   }
 }
 
+// 2. Load per-module configuration files.
 Collection urls = PathScanner.getConfigURLs();
 logger.debug("Loading configs at the following URLs {}", urls);
 for (URL url : urls) {
+  logger.debug("Loading module config. file at {}.", url);
   fallback = ConfigFactory.parseURL(url).withFallback(fallback);
 }
 
-Config effectiveConfig = 
ConfigFactory.load(overrideFileName).withFallback(fallback);
+// 3. Load any overrides configuration file and overrides from JVM
--- End diff --

Reworded a bit.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r35053195
  
--- Diff: 
common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -44,31 +46,37 @@
 import com.google.common.collect.ImmutableList;
 import com.typesafe.config.Config;
 import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigRenderOptions;
 
 public final class DrillConfig extends NestedConfig{
-//  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
+  private static final Logger logger = getLogger(DrillConfig.class);
+
   private final ObjectMapper mapper;
   private final ImmutableList startupArguments;
 
   public static final boolean ON_OSX = 
System.getProperty("os.name").contains("OS X");
 
-  @SuppressWarnings("restriction")  private static final long 
MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();
+  @SuppressWarnings("restriction")
+  private static final long MAX_DIRECT_MEMORY = 
sun.misc.VM.maxDirectMemory();
 
   @SuppressWarnings("unchecked")
   private volatile List> sinkQueues = new 
CopyOnWriteArrayList>(new Queue[1]);
 
   @VisibleForTesting
-  public DrillConfig(Config config, boolean enableServer) {
+  public DrillConfig(Config config, boolean enableServerConfigs) {
 super(config);
-logger.debug("Setting up config object.");
+logger.debug("Setting up DrillConfig object.");
+if (logger.isTraceEnabled()) {
--- End diff --

No, I didn't expect it to take any significant time; I just thought someone 
would object if the calls weren't guarded by that if statement.

So ... remove it to reduce the clutter, or keep/leave 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r35053589
  
--- Diff: 
common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -115,19 +124,26 @@ public static DrillConfig forClient() {
* 
* Configuration values are retrieved as follows:
* 
-   * Check a single copy of "drill-override.conf". If multiple copies 
are on the classpath, behavior is
-   * indeterminate.  If a non-null value for overrideFileName is provided, 
this is utilized instead of drill-override.conf.
-   * Check all copies of "drill-module.conf". Loading order is 
indeterminate.
-   * Check a single copy of "drill-default.conf". If multiple copies 
are on the classpath, behavior is
-   * indeterminate.
+   * Check a single copy of "drill-override.conf".  If multiple copies 
are
+   * on the classpath, which copy is read is indeterminate.
+   * If a non-null value for overrideFileResourcePathname is provided, 
this
+   * is used instead of "{@code drill-override.conf}".
+   * Check all copies of "{@code drill-module.conf}".  Loading order is
+   * indeterminate.
+   * Check a single copy of "{@code drill-default.conf}".  If multiple
+   * copies are on the classpath, which copy is read is 
indeterminate.
* 
*
* 
-   *  @param overrideFileName The name of the file to use for override 
purposes.
+   * @param overrideFileResourcePathname
+   *  the classpath resource pathname of the file to use for
+   *  configuration override purposes; {@code null} specifies to 
use the
+   *  default pathname ({@link CommonConstants.CONFIG_OVERRIDE})
+   *  (not to not look for an override file at 
all)
--- End diff --

Reworded.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r35053796
  
--- Diff: 
common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java
 ---
@@ -27,11 +27,20 @@
   static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(FormatPluginConfigBase.class);
 
 
-  public synchronized static Class[] getSubTypes(DrillConfig config){
-List packages = 
config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
-Class[] sec = 
PathScanner.scanForImplementationsArr(FormatPluginConfig.class, packages);
-logger.debug("Adding Format Plugin Configs including {}", (Object) sec 
);
-return sec;
+  public synchronized static Class[] getSubTypes(DrillConfig config) {
+List packages =
+
config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
+Class[] pluginClasses =
+PathScanner.scanForImplementationsArr(FormatPluginConfig.class, 
packages);
+if (logger.isDebugEnabled()) {
+  final StringBuilder sb = new StringBuilder();
--- End diff --

Yeah, I thought it should be factored out, but it wasn't clear where.   
(Also, the functionality is close to that of some utility join(...) 
methods--but not quite the same.)




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r35054916
  
--- Diff: 
common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +154,83 @@ public static DrillConfig create(Properties 
testConfigurations) {
 return create(null, testConfigurations, true);
   }
 
-  public static DrillConfig create(String overrideFileName, boolean 
enableServerConfigs) {
-return create(overrideFileName, null, enableServerConfigs);
+  /**
+   * ...
+   * @param overrideFileResourcePathname
+   *  see {@link #create(String)}'s {@code 
overrideFileResourcePathname}
+   */
+  public static DrillConfig create(String overrideFileResourcePathname, 
boolean enableServerConfigs) {
+return create(overrideFileResourcePathname, null, enableServerConfigs);
   }
 
-  private static DrillConfig create(String overrideFileName, Properties 
overriderProps, boolean enableServerConfigs) {
-overrideFileName = overrideFileName == null ? 
CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
-// first we load defaults.
+  /**
+   * ...
--- End diff --

[Reply via e-mail because I don't currently see this comment of yours on 
the pull-request page.]

> > +  /**
> > +   * ...xx

 > I haven't seen this convention before, but I assume this means that the 
docs for similar methods suffice and don't need to be repeated here.

No, actually it was an attempt to indicate that I hadn't addressed the 
method description (the part before the parameters), and not intended to mean 
"otherwise same as above".

I was trying to quickly at least document the parameter (add the reference 
to the other existing text) without taking time to fill in the whole 
documentation comment.

Would simply leaving the method description part blank be better than using 
"..."?

Daniel





Jason Altekruse wrote:
>
> In common/src/main/java/org/apache/drill/common/config/DrillConfig.java 
<https://github.com/apache/drill/pull/93#discussion_r35042277>:
>
> >}
> >
> > -  private static DrillConfig create(String overrideFileName, 
Properties overriderProps, boolean enableServerConfigs) {
> > -overrideFileName = overrideFileName == null ? 
CommonConstants.CONFIG_OVERRIDE : overrideFileName;
> > -
> > -// first we load defaults.
> > +  /**
> > +   * ...
>
> I haven't seen this convention before, but I assume this means that the 
docs for similar methods suffice and don't need to be repeated here. I would 
possibly recommend moving the longer comment above to here, in which case I 
think it would be possible to just put "..." above all of the alternative 
implementations of this method. I think it makes the most sense to put the 
comment describing the general idea of the functions purpose above the version 
that also describes what all of parameters can do.
>
> —
> Reply to this email directly or view it on GitHub 
<https://github.com/apache/drill/pull/93/files#r35042277>.
>


-- 
Daniel Barclay
MapR Technologies




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r35136041
  
--- Diff: 
common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +154,76 @@ public static DrillConfig create(Properties 
testConfigurations) {
 return create(null, testConfigurations, true);
   }
 
-  public static DrillConfig create(String overrideFileName, boolean 
enableServerConfigs) {
-return create(overrideFileName, null, enableServerConfigs);
+  /**
+   * ...
+   * @param overrideFileResourcePathname
+   *  see {@link #create(String)}'s {@code 
overrideFileResourcePathname}
+   */
+  public static DrillConfig create(String overrideFileResourcePathname, 
boolean enableServerConfigs) {
+return create(overrideFileResourcePathname, null, enableServerConfigs);
   }
 
-  private static DrillConfig create(String overrideFileName, Properties 
overriderProps, boolean enableServerConfigs) {
-overrideFileName = overrideFileName == null ? 
CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
-// first we load defaults.
+  /**
+   * ...
+   * @param overrideFileResourcePathname
+   *  see {@link #create(String)}'s {@code 
overrideFileResourcePathname}
+   * @param overriderProps
+   *  optional property map for further overriding (after override 
file
+   *  is assimilated
+   * @param enableServerConfigs
+   *  whether to enable server-specific configuration options
+   * @return
+   */
+  private static DrillConfig create(String overrideFileResourcePathname,
+final Properties overriderProps,
+final boolean enableServerConfigs) {
+overrideFileResourcePathname =
+overrideFileResourcePathname == null
+? CommonConstants.CONFIG_OVERRIDE
+: overrideFileResourcePathname;
+
+// 1. Load defaults configuration file.
 Config fallback = null;
 final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
 for (ClassLoader classLoader : classLoaders) {
-  if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) 
{
-fallback = ConfigFactory.load(classLoader, 
CommonConstants.CONFIG_DEFAULT);
+  final URL url =
+  classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+  if (null != url) {
+logger.debug("Loading base configuration file at {}.", url);
+fallback =
+ConfigFactory.load(classLoader,
+   CommonConstants.CONFIG_DEFAULT);
 break;
   }
 }
 
+// 2. Load per-module configuration files.
 Collection urls = PathScanner.getConfigURLs();
-logger.debug("Loading configs at the following URLs {}", urls);
 for (URL url : urls) {
+  logger.debug("Loading module configuration file at {}.", url);
--- End diff --

Done.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r35136191
  
--- Diff: 
common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -44,31 +46,37 @@
 import com.google.common.collect.ImmutableList;
 import com.typesafe.config.Config;
 import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigRenderOptions;
 
 public final class DrillConfig extends NestedConfig{
-//  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
+  private static final Logger logger = getLogger(DrillConfig.class);
+
   private final ObjectMapper mapper;
   private final ImmutableList startupArguments;
 
   public static final boolean ON_OSX = 
System.getProperty("os.name").contains("OS X");
 
-  @SuppressWarnings("restriction")  private static final long 
MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();
+  @SuppressWarnings("restriction")
+  private static final long MAX_DIRECT_MEMORY = 
sun.misc.VM.maxDirectMemory();
 
   @SuppressWarnings("unchecked")
   private volatile List> sinkQueues = new 
CopyOnWriteArrayList>(new Queue[1]);
 
   @VisibleForTesting
-  public DrillConfig(Config config, boolean enableServer) {
+  public DrillConfig(Config config, boolean enableServerConfigs) {
 super(config);
-logger.debug("Setting up config object.");
+logger.debug("Setting up DrillConfig object.");
+if (logger.isTraceEnabled()) {
--- End diff --

Removed isTraceEnabled guard.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r35279254
  
--- Diff: 
common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +154,83 @@ public static DrillConfig create(Properties 
testConfigurations) {
 return create(null, testConfigurations, true);
   }
 
-  public static DrillConfig create(String overrideFileName, boolean 
enableServerConfigs) {
-return create(overrideFileName, null, enableServerConfigs);
+  /**
+   * ...
+   * @param overrideFileResourcePathname
+   *  see {@link #create(String)}'s {@code 
overrideFileResourcePathname}
+   */
+  public static DrillConfig create(String overrideFileResourcePathname, 
boolean enableServerConfigs) {
+return create(overrideFileResourcePathname, null, enableServerConfigs);
   }
 
-  private static DrillConfig create(String overrideFileName, Properties 
overriderProps, boolean enableServerConfigs) {
-overrideFileName = overrideFileName == null ? 
CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
-// first we load defaults.
+  /**
+   * ...
--- End diff --

>  I'm not even sure that it makes sense to document these methods 
individually. 

If a method is not documented individually, then viewing that method's 
documentation isn't going to yield much information--remember that people don't 
see comments just in the context of the surrounding source code, but also in 
IDEs (e.g., showing one method's documentation at a time) and in generated 
Javadoc pages.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3153: Fix JDBC's getIdentifierQuoteStrin...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/99

DRILL-3153: Fix JDBC's getIdentifierQuoteString() to report Drill's 
backquote.

Added override of Avatica's default implementation returning SQL standard 
value.  Added Javadoc.  Added unit test.

Also moved unit test for nullsAreSortedXxx methods.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill 
bugs/drill-3153-getIdentifierQuoteString

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/99.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 #99


commit 299fc974aca30e20cda65102b83fd7f071105e27
Author: dbarclay 
Date:   2015-07-24T00:46:05Z

DRILL-3153: Fix JDBC's getIdentifierQuoteString() to report Drill's 
backquote.

Added override of Avatica's default implementation returning SQL
standard value.  Added Javadoc.  Added unit test.

Also moved unit test for nullsAreSortedXxx methods.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/93#issuecomment-125272213
  
(More "final" added in already touched methods.)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/93#discussion_r35559349
  
--- Diff: 
common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java
 ---
@@ -27,11 +27,20 @@
   static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(FormatPluginConfigBase.class);
 
 
-  public synchronized static Class[] getSubTypes(DrillConfig config){
-List packages = 
config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
-Class[] sec = 
PathScanner.scanForImplementationsArr(FormatPluginConfig.class, packages);
-logger.debug("Adding Format Plugin Configs including {}", (Object) sec 
);
-return sec;
+  public synchronized static Class[] getSubTypes(DrillConfig config) {
+List packages =
+
config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
+Class[] pluginClasses =
+PathScanner.scanForImplementationsArr(FormatPluginConfig.class, 
packages);
+if (logger.isDebugEnabled()) {
+  final StringBuilder sb = new StringBuilder();
--- End diff --

Reduce line count by factoring out part that Joiner could be used for.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/101

DRILL-3566: Fix:  PreparedStatement.executeQuery() got ClassCastException

Main:
Restored DrillResultSetImpl(...)'s statement parameter from overly
restrictive DrillStatementImpl to AvaticaStatement and removed caller
cast that was throwing.  (Relatedly, adjusted getStatement() and moved
internal casting from statement to connection.)

Added basic test of querying via Prepared Statement.  
[PreparedStatementTest]
Added some case test of statement-creation methods.  [Connection Test]

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill bugs/drill-3566

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/101.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 #101


commit a3b979734b0ead26e3fb903104d9b65f8464b663
Author: dbarclay 
Date:   2015-07-28T02:27:50Z

DRILL-3566: Fix:  PreparedStatement.executeQuery() got ClassCastException.

Main:
Restored DrillResultSetImpl(...)'s statement parameter from overly
restrictive DrillStatementImpl to AvaticaStatement and removed caller
cast that was throwing.  (Relatedly, adjusted getStatement() and moved
internal casting from statement to connection.)

Added basic test of querying via PreparedStatement.  [PreparedStatementTest]
Added some case test of statement-creation methods.  [ConnectionTest]




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/93


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3566: PreparedStatement fix and DRILL-33...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/111

DRILL-3566: PreparedStatement fix and DRILL-3347: ...hadoop.io.Text fix



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill 
bugs/drill-3566_etc._jdbc

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/111.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 #111


commit aad3db5dd3787e0c0e26b42e02a2351d1d5df788
Author: dbarclay 
Date:   2015-07-28T02:27:50Z

DRILL-3566: Fix:  PreparedStatement.executeQuery() got ClassCastException.

Main:
Restored DrillResultSetImpl(...)'s statement parameter from overly
restrictive DrillStatementImpl to AvaticaStatement and removed caller
cast that was throwing.  (Relatedly, adjusted getStatement() and moved
internal casting from statement to connection.)

Added basic test of querying via PreparedStatement.  [PreparedStatementTest]
Added some case test of statement-creation methods.  [ConnectionTest]

commit 537f7db912f8a80088b331bde67cefbc132fae5a
Author: dbarclay 
Date:   2015-08-04T23:51:07Z

DRILL-3347: VARCHAR ResultSet.getObject returned ...hadoop.io.Text, not 
String.

Core fix:
- Fixed {,Nullable}VarCharAccessor's getObject() to return String instead of
  value vector's internal org.apache.hadoop.io.Text.
- Updated unit tests (to expect only String now).
  [DatabaseMetaDataGetColumnsTest, ResultSetMetaDataTest]

Also Added getObject check in tracing proxy test.  [TracingProxyDriverTest]
Changed hard references to Hadoop's Text and JodaTime's Period to strings in
warning check in tracing proxy.  [InvocationReporterImpl]

Cleanup:
- Added @Override annotations.  [SqlAccessors]
- (Unintentionally) fixed (undetected) missing comma.  
[ValueVectorTypes.tdd]




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/101


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3641: Doc. RecordBatch.IterOutcome (enum...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/113

DRILL-3641: Doc. RecordBatch.IterOutcome (enumerators and possible 
sequences).

Documented RecordBatch.IterOutcome (RecordBatch.next() protocol) much more.

Also moved AbstractRecordBatch.BatchState's documentation test from
non-documentation comments to documentation comments.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill bugs/drill-3641

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/113.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 #113


commit 5ca94981442ea0a899c690ff1471369618a5471d
Author: dbarclay 
Date:   2015-07-31T01:54:59Z

DRILL-3641: Doc. RecordBatch.IterOutcome (enumerators and possible 
sequences).

Documented RecordBatch.IterOutcome (RecordBatch.next() protocol) much more.

Also moved AbstractRecordBatch.BatchState's documentation test from
non-documentation comments to documentation comments.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3641: Doc. RecordBatch.IterOutcome (enum...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/113#issuecomment-131672321
  
> Would be good to note here the purpose. ...

Done.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3641: Doc. RecordBatch.IterOutcome (enum...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/113#issuecomment-131674681
  
Hey, what happens (what is a caller supposed to do) if the caller receives 
NOT_YET but the caller has no local work to do?

(Re-calling next() immediately would yield a spin wait, so presumably we 
avoid that if possible.

A timed wait would involve a (probably) arbitrary time, so presumably we 
avoid that if possible.

(Delegating to the next-level caller by returning NOT_YET just devolves 
back to this same question.)


Is there another general option (general in the sense of callers and 
callees that don't know anything about each other)?

Or is it that NOT_YET is used only between pairs of callers and callee that 
know enough about each other to coordinate somehow so they can avoid spin waits 
and aribitrary delays?)




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/116#issuecomment-131970908
  
It seems that you have shaded references to every Java package other than 
org.apache.drill.** (not just for every package from every depended-on 
artifact, but for _every_ package--including JDK packages).

Note how the following command says it can't find "oadd/java/sql/Driver":

$ java -cp ./exec/jdbc-all/target/drill-jdbc-all-1.2.0-SNAPSHOT.jar 
org.apache.drill.jdbc.Driver
Exception in thread "main" java.lang.NoClassDefFoundError: 
oadd/java/sql/Driver
...

It seems that in trying to load org.apache.drill.jdbc.Driver it tried to 
load its base class, normally java.sql.Driver from the JDK, but the reference 
to that base class was shaded to oadd.org.java.sql.Driver, so of course it 
couldn't be found.

(The expected behavior of that command is an error that 
org.apache.drill.jdbc.Driver doesn't have a main method.)


What setup did you use to test the shading?



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3548: Add/edit various Javadoc(/etc.) (a...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/118

DRILL-3548: Add/edit various Javadoc(/etc.) (assorted from plugin 
exploration).

Added/edited various Javadoc and code comments.

Also:
Fixed Javadoc references ("#", imports).
Edited a few messages.
Wrapped a few lines.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill 
bugs/drill-3548_various_doc

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/118.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 #118


commit 90fa3476ab4b988a93b8720219ea3856c7a250dd
Author: dbarclay 
Date:   2015-07-20T20:50:30Z

DRILL-3548: Add/edit various Javadoc(/etc.) (assorted from plugin 
exploration).

Added/edited various Javadoc and code comments.

Also:
Fixed Javadoc references ("#", imports).
Edited a few messages.
Wrapped a few lines.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3661: Edit JDBC doc. for clarity, more c...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/119

DRILL-3661: Edit JDBC doc. for clarity, more consistency, bug fixes.

Also fixed two serialVersionUID literals.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill bugs/drill-3661_JDBC_doc

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/119.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 #119


commit b0e76a58a4573f6924ecadaa9a649af5f878d5a2
Author: dbarclay 
Date:   2015-08-18T00:26:45Z

DRILL-3661: Edit JDBC doc. for clarity, more consistency, bug fixes.

Also fixed two serialVersionUID literals.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3661: Edit JDBC doc. for clarity, more c...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/119#discussion_r38443172
  
--- Diff: 
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillConnectionConfig.java ---
@@ -22,6 +22,24 @@
 
 import net.hydromatic.avatica.ConnectionConfigImpl;
 
+
+// TODO(DRILL-):  Change public DrillConnectionConfig from class to 
interface.
--- End diff --

Fixed.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/116#issuecomment-136889943
  
Something seems to be broken.

After rebasing your branch on my branch with my DRILL-3347 (Hadoop Test) 
and DRILL-3566 (Prep.Stmt.) fixes, I tried installing the resulting JDBC-all 
Jar file on Spotfire, but Spotfire's getting IndexOutOfBoundsExceptions 
somewhere within ResultSet.next().

I'll see if I can identify which shading might be causing that or what's 
going on in next().



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/116#issuecomment-136890582
  
Another problem is that Logback logging might be broken (although maybe I 
just configured it wrong):

When I added (on Spotfire's class path) a Jar file containing a logback.xml 
file in order to turn on logging, instead of getting log messages, I got these 
messages:

SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further 
details.

My guess is that whatever module we intended to provide 
org.slf4j.impl.StaticLoggerBinder isn't included in the JDBC-all Jar file now.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/116#issuecomment-136902647
  
Regarding SLF4J and what should or shouldn't be included in the JDBC-all 
Jar file:

Do we intend that someone using the JDBC-all Jar file can turn on Drill 
JDBC-layer logging by adding to the class path only a logback.xml file (that 
is, without adding additional Jar files/classes)?  If so, then it seems like 
the JDBC-all Jar file is probably missing the logback-classic Jar file (which 
contains a org.slf4j.impl.StaticLoggerBinder).

Or is it the case that we're leaving out back-end SLF4J libraries/classes 
on purpose (perhaps to avoid interfering with the user's choice of which back 
end to use)?

 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3661: Edit JDBC doc. for clarity, more c...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/119#discussion_r38558416
  
--- Diff: 
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillCursor.java ---
@@ -66,17 +69,18 @@
   private boolean afterFirstBatch = false;
 
   /**
-   * Whether the next call to this.next() should just return {@code true} 
rather
-   * than calling nextRowInternally() to try to advance to the next
-   * record.
+   * Whether the next call to {@code this.}{@link #next()} should just 
return
+   * {@code true} rather than calling {@link #nextRowInternally()} to try 
to
+   * advance to the next record.
* 
-   *   Currently, can be true only for first call to next().
+   *   Currently, can be true only for first call to {@link #next()}.
* 
* 
-   *   (Relates to loadInitialSchema()'s calling nextRowInternally()
+   *   (Relates to {@link #loadInitialSchema()}'s calling 
nextRowInternally()
--- End diff --

Frequently, for second and later references in the same paragraph or 
adjacent paragraphs, I avoid making those later references links, since having 
more links can make the text less readable.

However, since here I already have multiple links to next(), I did make 
that nextRowInternally() a link (and added a missing {@code...} for 
"Statement.execute...(...)").


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3661: Edit JDBC doc. for clarity, more c...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/119#issuecomment-137176204
  
Rebased.  Edited doc. comment formatting.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3566: PreparedStatement fix and DRILL-33...

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/111


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/143

DRILL-3566: Fix:  PreparedStatement.executeQuery() got ClassCastException.

Main:
Restored DrillResultSetImpl(...)'s statement parameter from overly
restrictive DrillStatementImpl to AvaticaStatement and removed caller
cast that was throwing.  (Relatedly, adjusted getStatement() and moved
internal casting from statement to connection.)

Added basic test of querying via PreparedStatement.  [PreparedStatementTest]
Added some case test of statement-creation methods.  [ConnectionTest]

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill bugs/drill-3566

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/143.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 #143


commit 1a870538c66fa59070facce7e2a4342d9869b51e
Author: dbarclay 
Date:   2015-07-28T02:27:50Z

DRILL-3566: Fix:  PreparedStatement.executeQuery() got ClassCastException.

Main:
Restored DrillResultSetImpl(...)'s statement parameter from overly
restrictive DrillStatementImpl to AvaticaStatement and removed caller
cast that was throwing.  (Relatedly, adjusted getStatement() and moved
internal casting from statement to connection.)

Added basic test of querying via PreparedStatement.  [PreparedStatementTest]
Added some case test of statement-creation methods.  [ConnectionTest]




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3347: VARCHAR ResultSet.getObject return...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/144

DRILL-3347: VARCHAR ResultSet.getObject returned ...hadoop.io.Text, not 
String.


Core fix:
- Fixed {,Nullable}VarCharAccessor's getObject() to return String instead of
  value vector's internal org.apache.hadoop.io.Text.
- Updated unit tests (to expect only String now).
  [DatabaseMetaDataGetColumnsTest, ResultSetMetaDataTest]

Also Added getObject check in tracing proxy test.  [TracingProxyDriverTest]
Changed hard references to Hadoop's Text and JodaTime's Period to strings in
warning check in tracing proxy.  [InvocationReporterImpl]

Cleanup:
- Added @Override annotations.  [SqlAccessors]
- (Unintentionally) fixed (undetected) missing comma.  
[ValueVectorTypes.tdd]

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill bugs/drill-3347

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/144.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 #144


commit ccd6ded4387b2b27849762f607a13fa4236351b6
Author: dbarclay 
Date:   2015-08-04T23:51:07Z

DRILL-3347: VARCHAR ResultSet.getObject returned ...hadoop.io.Text, not 
String.

Core fix:
- Fixed {,Nullable}VarCharAccessor's getObject() to return String instead of
  value vector's internal org.apache.hadoop.io.Text.
- Updated unit tests (to expect only String now).
  [DatabaseMetaDataGetColumnsTest, ResultSetMetaDataTest]

Also Added getObject check in tracing proxy test.  [TracingProxyDriverTest]
Changed hard references to Hadoop's Text and JodaTime's Period to strings in
warning check in tracing proxy.  [InvocationReporterImpl]

Cleanup:
- Added @Override annotations.  [SqlAccessors]
- (Unintentionally) fixed (undetected) missing comma.  
[ValueVectorTypes.tdd]




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/143#discussion_r38708736
  
--- Diff: 
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillJdbc41Factory.java ---
@@ -103,9 +103,9 @@ public DrillResultSetImpl newResultSet(AvaticaStatement 
statement,
  TimeZone timeZone) {
 final ResultSetMetaData metaData =
 newResultSetMetaData(statement, prepareResult.getColumnList());
-return new DrillResultSetImpl( (DrillStatementImpl) statement,
-   (DrillPrepareResult) prepareResult,
-   metaData, timeZone);
+return new DrillResultSetImpl(statement,
+  (DrillPrepareResult) prepareResult,
--- End diff --

Simplified.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/143#discussion_r38709955
  
--- Diff: 
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -850,9 +851,9 @@ public void moveToCurrentRow() throws SQLException {
   }
 
   @Override
-  public DrillStatementImpl getStatement() {
+  public AvaticaStatement getStatement() {
--- End diff --

One reason for leaving it in (after changing the return type back) is to 
hold that comment pointing out that this method doesn't call checkNotClosed() 
as most other methods do.

Fixed copy/paste/forgot-to-edit error "close()"  to "getStatement()" in 
comment.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/143#discussion_r38710714
  
--- Diff: 
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -87,12 +88,12 @@
   boolean hasPendingCancelationNotification;
 
 
-  DrillResultSetImpl(DrillStatementImpl statement, AvaticaPrepareResult 
prepareResult,
+  DrillResultSetImpl(AvaticaStatement statement, AvaticaPrepareResult 
prepareResult,
  ResultSetMetaData resultSetMetaData, TimeZone 
timeZone) {
 super(statement, prepareResult, resultSetMetaData, timeZone);
-this.statement = statement;
+connection = (DrillConnectionImpl) statement.getConnection();
 final int batchQueueThrottlingThreshold =
-this.getStatement().getConnection().getClient().getConfig().getInt(
+connection.getClient().getConfig().getInt(
--- End diff --

Yeah, I don't know why the code was like that.

Reworked a bit to eliminate redundant calls and code.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/143#discussion_r38710996
  
--- Diff: 
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -1334,8 +1335,6 @@ public String getQueryId() throws SQLException {
 
   @Override
   protected DrillResultSetImpl execute() throws SQLException{
-DrillConnectionImpl connection = (DrillConnectionImpl) 
statement.getConnection();
-
 connection.getClient().runQuery(QueryType.SQL, 
this.prepareResult.getSql(),
--- End diff --

I don't know why the code was like that.  (Maybe to be symmetric with 
connection.getDriver(), or maybe from before client was kept?)

Eliminated redundant call to getClient().


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/143#issuecomment-137607549
  
> can you confirm that this patch passes all our "jdbc" tests ?

It passed before the post-review update commit.  I'm re-running regular 
tests now.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/116#issuecomment-137653774
  
> Can you start by reviewing my branch? Let's get it right before we try 
rebasing.

I'm confused by that part of your comment.  I have been reviewing your 
branch, and I think I've reviewed the latest (with-test/without-ProGuard) 
version of it (hence my earlier review comments, and your replies to them).

Have I missed a message or am I looking at the wrong thing?  (I have been 
and am looking at this [Pull Request 
#116](https://github.com/apache/drill/pull/116).)

Also, your mention of rebasing sounds like a reference to a preceding 
request to rebase, but I don't see or recall any mention of rebasing your patch.


Looking again, I see that I just skimmed over the class loader that's just 
for the test.  Is it that you wanted more review of that class?

Oh--it is that I hadn't indicated that I was done reviewing it because I 
didn't give a +1 yet?  I hadn't done that before because I was waiting for 
confirmation on my SLF4J question.  You have answered that now (below), so the 
patch seems "plus-oneable" unless the resolution of whatever the confusion is 
here reveals a problem with the patch.


> With regards to slf4j and logging: ... We support any slf4j logging tool 
but we don't package any. ... 
> If we include a logging framework, then a user can't use their own for 
centralized logging...

Roger.  Yeah, I thought it might be that.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/143#issuecomment-137794418
  
> can you confirm that this patch passes all our "jdbc" tests ?

Yes, it passes our regular tests.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/116#issuecomment-137805930
  
Okay, now I think I see what happened.

First of all, you can now ignore that "Something ... broken ..." comment; 
it's obsolete.  (Your patch no longer seems broken.)

(At first I thought things didn't work, and added that comment.  Then I 
noticed that I had a local version/build mismatch, and amended the comment to 
say "hold on; I'm checking again," and tested again.  Then everything worked, 
so I just deleted the comment from the GitHub review.  Next time I'll amend 
rather than delete.)

And now I recognize the "rebasing" reference:  I didn't mean rebasing like 
rebasing on the latest version of master.  I was just mentioning that rebasing 
was what I happened to use (as opposed to cherry-picking, merging, or other 
patching), in case my choice there caused the apparent breakage, to apply 
patches for DRILL-3347 and DRILL-3566 so I could try you patch with Spotfire 
(with would have hit the DRILL-3347 and DRILL-3566 bugs).

So ...

Your patch seems good; Spotfire ran fine with 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/116#issuecomment-137811250
  
Would it be better if the JDBC-all Jar file didn't contain a logback.xml 
file?

If the JDBC-all Jar doesn't already have that file, then to configure 
logback differently with their own logback.xml file, users only have to get 
their own file on the classpath _somewhere_.

However, if the JDBC-all Jar does already have that file, it seems that 
users will have to get their logback.xml file on the class path _before_ the 
JDBC-all Jar file (right?).  Although sometimes that's trivial, when the class 
path is set by, say, listing all .jar files in a lib/ directory (e.g., as in 
Tomcat for Spotfire), controlling the order might not be possible or easy.

(A solution to keep the contents of the logback.xml file (as a convenient 
starting point for the user to copy out and modify) in the JDBC-all JAR file 
but not really have an active/interfering actual logback.xml file would be to 
use a modified name (e.g, logback-example.xml).)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/116#issuecomment-138646368
  
+1 (non-binding) once logback.xml is removed


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3778: Add missed part of DRILL-3160 (mak...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/158

DRILL-3778: Add missed part of DRILL-3160 (making JDBC Javadoc available).

Main:
Configured Javadoc generation (title, package groups, version in headers).
Added link to JDBC page in Drill documentation site.
Edited/fixed some JDBC Javadoc comments.
Added explicit SQLConversionOverflowException to throws clauses for Javadoc
effect.
Added some imports for Javadoc references.

Misc.:
Fixed a couple Javadoc syntax errors.
Fixed POM indentation.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill 
bugs/drill-3778_DRILL-3160_missed

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/158.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 #158


commit 24226aebf6274806e8d1a355f4c6b83dedee2e83
Author: dbarclay 
Date:   2015-09-14T17:54:34Z

DRILL-3778: Add missed part of DRILL-3160 (making JDBC Javadoc available).

Main:
Configured Javadoc generation (title, package groups, version in headers).
Added link to JDBC page in Drill documentation site.
Edited/fixed some JDBC Javadoc comments.
Added explicit SQLConversionOverflowException to throws clauses for Javadoc
effect.
Added some imports for Javadoc references.

Misc.:
Fixed a couple Javadoc syntax errors.
Fixed POM indentation.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3822: Have PathScanner use own, not thre...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/165

DRILL-3822:  Have PathScanner use own, not thread-context, class loader.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill bugs/drill-3822

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/165.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 #165


commit 605a65780a38da2e1e8e35c521876e68e5901cab
Author: dbarclay 
Date:   2015-09-23T01:04:45Z

DRILL-3822:  Have PathScanner use own, not thread-context, class loader.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3822: Have PathScanner use own, not thre...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/165#issuecomment-142719537
  
@jacques-n, can you review and merge this?

We (Krystal and I) have verified that this patch fixes the problem seen in 
SQuirrel.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3822: Have PathScanner use own, not thre...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/166

DRILL-3822:  Have PathScanner use own, not thread-context, class loader.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill bugs/drill-3822

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/166.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 #166


commit 605a65780a38da2e1e8e35c521876e68e5901cab
Author: dbarclay 
Date:   2015-09-23T01:04:45Z

DRILL-3822:  Have PathScanner use own, not thread-context, class loader.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3822: Have PathScanner use own, not thre...

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/165


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3822: Have PathScanner use own, not thre...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/166#issuecomment-142724811
  
@jacques-n, can you review and merge this?

We (Krystal and I) have verified that this patch fixes the problem seen in 
SQuirrel.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3778: Add missed part of DRILL-3160 (mak...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/158#issuecomment-142729309
  
@jaltekruse, can you review and merge this commit?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3822: Have PathScanner use own, not thre...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/166#issuecomment-142730429
  
@hakim, can you review and merge this (probably with your fix for 
DRILL-3874)?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3778: Add missed part of DRILL-3160 (mak...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/158#discussion_r40260511
  
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/package-info.java 
---
@@ -19,7 +19,13 @@
 /**
  * JDBC driver for Drill.
  * 
- *   Drill's JDBC driver class is {@link org.apache.drill.jdbc.Driver}.
+ *   Drill's JDBC driver class is
+ *   {@link org.apache.drill.jdbc.Driver org.apache.drill.jdbc.Driver}.
--- End diff --

Yes.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2489: Throw exception from remaining met...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/170

DRILL-2489: Throw exception from remaining methods for closed JDBC objects.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill 
bugs/drill-2769_2489_JDBC_exceptions

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/170.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 #170


commit 90bf53dee9407d40065a820a6f0b7a35cf54fbea
Author: dbarclay 
Date:   2015-08-19T23:55:40Z

DRILL-2489: Throw exception from remaining methods for closed JDBC objects.

Refactored unit test to check all methods per interface.  (Replaced 
individual,
static test methods with bulk reflection-based checking.)
[Drill2489CallsAfterCloseThrowExceptionsTest]

Added DrillResultSetMetaDataImpl.

Added method overrides to check state for remaining methods from Connection,
Statement, PreparedStatement, ResultSet, ResultSetMetaData and 
DatabaseMetaData.

Also:
- renamed checkNotClosed to throwIfClosed.

commit fb960d8be27c3a810474caedfe4a0890fd0222ac
Author: dbarclay 
Date:   2015-08-27T21:05:26Z

DRILL-2769: Fix most non-SQLException not-supported-yet exceptions.

Core:

Added (auto-scanning) unit test. 
[Drill2769UnsupportedReportsUseSqlExceptionTest]

Added translation of lots of UnsupportedOperationExceptions (and some
RuntimeExceptions) from Avatica code to SQLFeatureNotSupportedExceptions 
(tons
of method overrides).

Also:

Added explicit bounds checks in ResultSetMetaData methods and checking of
last-accessed column in DrillAccessorList.wasNull() (to fix other
RuntimeExceptions to SQLExceptions).

Added resetting of last-accessed column to fix latent bug in 
DrillAccessorList.

Hygiene:
- Renamed some zero-based index/ordinal-position parameters to "...Offset".
- Renamed some one-based index/ordinal-position parameters to "...Number".
- Renamed DrillAccessorList lastColumn to rowLastColumnOffset; declared
  explicit logical null value for rowLastColumnOffset.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2489: Throw exception from remaining met...

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/170


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: Bugs/drill 2769 2489 jdbc exceptions

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/171

Bugs/drill 2769 2489 jdbc exceptions

DRILL-2489: Throw exception from remaining methods for closed JDBC objects.
DRILL-2769: Fix most non-SQLException not-supported-yet exceptions.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill 
bugs/drill-2769_2489_JDBC_exceptions

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/171.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 #171


commit 90bf53dee9407d40065a820a6f0b7a35cf54fbea
Author: dbarclay 
Date:   2015-08-19T23:55:40Z

DRILL-2489: Throw exception from remaining methods for closed JDBC objects.

Refactored unit test to check all methods per interface.  (Replaced 
individual,
static test methods with bulk reflection-based checking.)
[Drill2489CallsAfterCloseThrowExceptionsTest]

Added DrillResultSetMetaDataImpl.

Added method overrides to check state for remaining methods from Connection,
Statement, PreparedStatement, ResultSet, ResultSetMetaData and 
DatabaseMetaData.

Also:
- renamed checkNotClosed to throwIfClosed.

commit fb960d8be27c3a810474caedfe4a0890fd0222ac
Author: dbarclay 
Date:   2015-08-27T21:05:26Z

DRILL-2769: Fix most non-SQLException not-supported-yet exceptions.

Core:

Added (auto-scanning) unit test. 
[Drill2769UnsupportedReportsUseSqlExceptionTest]

Added translation of lots of UnsupportedOperationExceptions (and some
RuntimeExceptions) from Avatica code to SQLFeatureNotSupportedExceptions 
(tons
of method overrides).

Also:

Added explicit bounds checks in ResultSetMetaData methods and checking of
last-accessed column in DrillAccessorList.wasNull() (to fix other
RuntimeExceptions to SQLExceptions).

Added resetting of last-accessed column to fix latent bug in 
DrillAccessorList.

Hygiene:
- Renamed some zero-based index/ordinal-position parameters to "...Offset".
- Renamed some one-based index/ordinal-position parameters to "...Number".
- Renamed DrillAccessorList lastColumn to rowLastColumnOffset; declared
  explicit logical null value for rowLastColumnOffset.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3848: Increase timeout time on several t...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/174

DRILL-3848:  Increase timeout time on several tests that time out 
frequently.

Tests:
- TestTpchDistributedConcurrent
- TestExampleQueries
- TestFunctionsQuery

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill 
bugs/drill-3848_extend_test_timeouts

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/174.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 #174


commit 1ac417dca34f42f0f33c07795665c0579810417d
Author: dbarclay 
Date:   2015-09-27T20:43:57Z

DRILL-3848:  Increase timeout time on several tests that time out 
frequently.

Tests:
- TestTpchDistributedConcurrent
- TestExampleQueries
- TestFunctionsQuery




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2961: Throw SQLException when attempting...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/71#issuecomment-143905747
  
This has been merged (commits 1dcf6cfb0d52435bcc7d1f4eada48fc0ce366dc4 and 
a3ec52a721860a966dfa351f719458a200b27cbf).


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2961: Throw SQLException when attempting...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/71#issuecomment-143908786
  
@kkhatua, Can you close this pull request?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix: No OK_NEW_SCHEMA for 0-row so...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/175

DRILL-2288: Fix: No OK_NEW_SCHEMA for 0-row source; missing downstream 
schema.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill 
bugs/drill-2288-3641-3659_no-rows_schema

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/175.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 #175


commit eee7c29785b1ae6545c2e02aa7676aa4509e1963
Author: dbarclay 
Date:   2015-09-27T20:55:21Z

DRILL-3641:  Doc. RecordBatch.IterOutcome (enumerators and possible 
sequences).  [RecordBatch, AbstractRecordBatch]

Documented RecordBatch.IterOutcome (RecordBatch.next() protocol) much more.

Also moved AbstractRecordBatch.BatchState's documentation text from
non-documentation comments to documentation comments.

commit ec5a9d0af8647af1472591549be1a9964f6a9151
Author: dbarclay 
Date:   2015-09-27T20:49:45Z

DRILL-2288:  Fix: No OK_NEW_SCHEMA for 0-row source; missing downstream 
schema.

Core fix:
- Fixed ScanBatch.Mutator's isNewSchema() to stop reporting "new schema"
  spuriously at end of JSON file with complex types.
- Fixed ScanBatch.next() to return OK_NEW_SCHEMA instead of NONE for certain
  case of zero-row source/reader with new schema.
- Fixed UnionAllRecordBatch to handle correct IterOutcome sequence from
  now-corrected ScanBatch.next().  (Was DRILL-3659.)

Also:
- Added unit test (checking involved metadata at JDBC level).
  [Drill2288GetColumnsMetadataWhenNoRowsTest, empty.json]
- Enhanced IteratorValidatorBatchIterator, mainly to validate IterOutcome
  value sequence from next().
- Documented SchemaChangeCallBack and renamed method for clarity.
  [SchemaChangeCallBack, ScanBatch, AbstractSingleRecordBatch]
- Other [ScanBatch]:
  - Added/edited various code and doc. comments.
  - Renamed boolean schemaChange -> schemaChanged.
  - Various code cleanup (e.g., "final", SuppressWarnings, whitespace).




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3589: Update JDBC driver to shade and mi...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/116#issuecomment-143913643
  
@jacques-n, can you close this pull request (given that that 
4e3b7dc0333a01e72d0ea9256331ea1e1dd51181 is in)?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix: No OK_NEW_SCHEMA for 0-row so...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/175#issuecomment-152326981
  
Closing in part to move to new branch.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix: No OK_NEW_SCHEMA for 0-row so...

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/175


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/228

DRILL-2288: Fix ScanBatch violation of IterOutcome protocol and downstream 
chain of bugs



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dsbos/incubator-drill bugs/drill-2288_etc

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/228.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 #228


commit faefc960f1a36cf25fa0b553bab75e1c3fc71222
Author: dbarclay 
Date:   2015-10-28T02:25:25Z

2288:  Pt. 1 Core:  Added unit test.  
[Drill2288GetColumnsMetadataWhenNoRowsTest, empty.json]

commit 396a41b155d1ad9413897ae4d87db2337642a1ea
Author: dbarclay 
Date:   2015-11-01T03:36:12Z

2288:  Pt. 1 Core:  Changed HBase test table #1's # of regions from 1 to 2. 
 [HBaseTestsSuite]

Also added TODO(DRILL-3954) comment about # of regions.

commit a0fe6b0787c284cda2006355e507c7caa6cae2a7
Author: dbarclay 
Date:   2015-10-28T02:35:11Z

2288:  Pt. 2 Core:  Documented IterOutcome much more clearly.  [RecordBatch]

Also edited some related Javadoc.

commit 24b3f4df90711b630b30fe4e2ad68ff798e5731a
Author: dbarclay 
Date:   2015-10-28T02:41:04Z

2288:  Pt. 2 Hyg.:  Edited doc., added @Override, etc.  
[AbstractRecordBatch, RecordBatch]

Purged unused SetupOutcome.
Added @Override.
Edited comments.
Fix some comments to doc. comments.

commit a3108ab18e5f99e9d18f57786b2efa717a61c432
Author: dbarclay 
Date:   2015-10-28T03:00:26Z

2288:  Pt. 3 Core&Hyg.:  Added validation of IterOutcome sequence.  
[IteratorValidatorBatchIterator]

Also:
Renamed internal members for clarity.
Added comments.

commit c8fc4b3f5c3df5871a9b07b8f4ae800ddbe0ce64
Author: dbarclay 
Date:   2015-10-28T03:31:14Z

2288:  Pt. 4 Core:  Fixed a NONE -> OK_NEW_SCHEMA in ScanBatch.next().  
[ScanBatch]

(With nearby comments.)

commit fcb1438724df83865a83749b097ca19d21cec444
Author: dbarclay 
Date:   2015-10-28T03:56:33Z

2288:  Pt. 4 Hyg.:  Edited comments, reordered, whitespace.  [ScanBatch]

Reordered
Added comments.
Aligned.

commit 9520345aae524d246876a744a8a676e00b0294dc
Author: dbarclay 
Date:   2015-10-28T04:02:25Z

2288:  Pt. 4 Core+:  Fixed UnionAllRecordBatch to receive IterOutcome 
sequence right.  (3659)  [UnionAllRecordBatch]

commit 3acf8ec8927974901d825859c8f00d1382aa2d87
Author: dbarclay 
Date:   2015-10-28T04:05:01Z

2288:  Pt. 5 Core:  Fixed ScanBatch.Mutator.isNewSchema() to stop spurious 
"new schema" reports (fix short-circuit OR, to call resetting method right).  
[ScanBatch]

commit 59ede9bda0e73c0bb6840018781f1f404d73f85c
Author: dbarclay 
Date:   2015-10-28T04:11:55Z

2288:  Pt. 5 Hyg.:  Renamed, edited comments, reordered.  [ScanBatch, 
SchemaChangeCallBack, AbstractSingleRecordBatch]

Renamed getSchemaChange -> getSchemaChangedAndReset.
Renamed schemaChange -> schemaChanged.
Added doc. comments.
Aligned.

commit d93dc1ee518db409e72b8672679640be65ae7a62
Author: dbarclay 
Date:   2015-10-28T04:20:47Z

2288:  Pt. 6 Core:  Avoided dummy Null.IntVec. column in JsonReader when 
not needed (MapWriter.isEmptyMap()).  [JsonReader, 3 vector files]

commit f989229ac5510273698af19d87b50044efed81c5
Author: dbarclay 
Date:   2015-10-28T04:32:44Z

2288:  Pt. 6 Hyg.:  Edited comments, message.  Fixed message formatting.  
[RecordReader, JSONFormatPlugin, JSONRecordReader, AbstractMapVector, 
JsonReader]

Fixed message formatting.
Edited comments.
Edited message.
Fixed spurious line break.

commit a360e9d0d7bfac0aa75831c35585f8ea890858dc
Author: dbarclay 
Date:   2015-10-28T05:06:13Z

2288:  Pt. 7 Core:  Added column families in HBaseRecordReader* to avoid 
dummy Null.IntVec. clash.  [HBaseRecordReader]

commit 18467a9ce14f6328b4766541267183927ebcdc9c
Author: dbarclay 
Date:   2015-10-28T05:06:52Z

2288:  Pt. 8 Core.1:  Cleared recordCount in 
OrderedPartitionRecordBatch.innerNext().  [OrderedPartitionRecordBatch]

commit 73bf71fc4af8e54599e908abbb993a64b066c097
Author: dbarclay 
Date:   2015-10-28T05:07:14Z

2288:  Pt. 8 Core.2:  Cleared recordCount in ProjectRecordBatch.innerNext.  
[ProjectRecordBatch]

commit 064187d1c23f2b4fc09e94066d66e79ef961c4f1
Author: dbarclay 
Date:   2015-10-28T05:08:22Z

2288:  Pt. 8 Core.3:  Cleared recordCount in TopNBatch.innerNext.  
[TopNBatch]

commit 8b9d1657ee22cee4432074f778e5d10e2c06e8e8
Author: dbarclay 
Date:   2015-10-28T05:24:35Z

2288:  Pt. 9 Core:  Had UnorderedReceiverBatch reset RecordBatchLoader's 
record count.  [UnorderedReceiverBatch, RecordBatchLoader]

commit 57c36854d6b65107d46b547fd9edd351c3149643
Author: dbarclay 
Date:   2015-10-29T00:28:50Z

2288:  Pt. 10 Core:  W

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43660011
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
 ---
@@ -93,13 +93,15 @@ public void ensureAtLeastOneField(ComplexWriter writer) 
{
 if (!atLeastOneWrite) {
   // if we had no columns, create one empty one so we can return some 
data for count purposes.
   SchemaPath sp = columns.get(0);
-  PathSegment root = sp.getRootSegment();
+  PathSegment fieldPath = sp.getRootSegment();
   BaseWriter.MapWriter fieldWriter = writer.rootAsMap();
-  while (root.getChild() != null && !root.getChild().isArray()) {
-fieldWriter = fieldWriter.map(root.getNameSegment().getPath());
-root = root.getChild();
+  while (fieldPath.getChild() != null && ! 
fieldPath.getChild().isArray()) {
+fieldWriter = 
fieldWriter.map(fieldPath.getNameSegment().getPath());
+fieldPath = fieldPath.getChild();
+  }
+  if (fieldWriter.isEmptyMap()) {
--- End diff --

That should be just a renaming:

It looked like the variable originally named "root" related to the root 
(from getRootSegment()) only until it was modified in the loop (so the name was 
misleading from the beginning of the loop and especially at the last reference).

Renaming it with "field" was to reflect that it corresponds to variable 
"fieldWriter" even as both are possibly modified in the loop.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43661248
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java
 ---
@@ -189,8 +189,8 @@ protected void putVector(String name, ValueVector 
vector) {
 Preconditions.checkNotNull(vector, "vector cannot be null")
 );
 if (old != null && old != vector) {
-  logger.debug("Field [%s] mutated from [%s] to [%s]", name, 
old.getClass().getSimpleName(),
-  vector.getClass().getSimpleName());
+  logger.debug("Field [{}] mutated from [{}] to [{}]", name, 
old.getClass().getSimpleName(),
--- End diff --



http://www.slf4j.org/api/org/slf4j/helpers/MessageFormatter.htmlDoesn't the 
number of brace replacement patterns correspond to the number of arguments?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43661592
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatch.java ---
@@ -23,60 +23,214 @@
 import org.apache.drill.exec.record.selection.SelectionVector4;
 
 /**
- * A record batch contains a set of field values for a particular range of 
records. In the case of a record batch
- * composed of ValueVectors, ideally a batch fits within L2 cache (~256k 
per core). The set of value vectors do not
- * change unless the next() IterOutcome is a *_NEW_SCHEMA type.
- *
- * A key thing to know is that the Iterator provided by record batch must 
align with the rank positions of the field ids
- * provided utilizing getValueVectorId();
+ * A record batch contains a set of field values for a particular range of
--- End diff --

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43664058
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java
 ---
@@ -335,7 +343,8 @@ public IterOutcome nextBatch() throws 
SchemaChangeException {
 return iterRight;
 
   default:
-throw new IllegalStateException(String.format("Unknown state 
%s.", iterRight));
+throw new IllegalStateException(
--- End diff --

Hmm.  It's been a while, so I don't recall exactly.  It looks like I 
wrapped it to fit within 80 columns since most of the code and comments just 
above fit within that width.  (Yes, now I see that the next throw statement 
below is also wider.)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43665769
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java
 ---
@@ -122,6 +122,7 @@ protected void killIncoming(final boolean sendUpstream) 
{
 
   @Override
   public IterOutcome innerNext() {
+recordCount = 0;
--- End diff --

That wouldn't have worked--the recordCount fields are declared in the 
subclasses, not in one of those superclasses. 

(I didn't try refactoring recordCount up into one of those base classes 
because I was already running into enough downstream problems every time I 
fixed an upstream increment.)





---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43666158
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java
 ---
@@ -325,10 +325,13 @@ public AggOutcome doWork() {
 if (EXTRA_DEBUG_1) {
   logger.debug("Received new schema.  Batch has {} 
records.", incoming.getRecordCount());
 }
-newSchema = true;
-this.cleanup();
-// TODO: new schema case needs to be handled appropriately
-return AggOutcome.UPDATE_AGGREGATOR;
+final BatchSchema newIncomingSchema = incoming.getSchema();
+if ((! newIncomingSchema.equals(schema)) && schema != 
null) {
--- End diff --

Yeah, I thought this change might not be right.

So what do I do (to at least avoid the schema-change exception that I was 
getting from one of the test suites, but do so without introducing the bug you 
point out)?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43704529
  
--- Diff: 
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseRecordReader.java
 ---
@@ -142,10 +148,12 @@ public void setup(OperatorContext context, 
OutputMutator output) throws Executio
   getOrCreateFamilyVector(column.getRootSegment().getPath(), 
false);
 }
   }
-  logger.debug("Opening scanner for HBase table '{}', Zookeeper quorum 
'{}', port '{}', znode '{}'.",
-  hbaseTableName, hbaseConf.get(HConstants.ZOOKEEPER_QUORUM),
-  hbaseConf.get(HBASE_ZOOKEEPER_PORT), 
hbaseConf.get(HConstants.ZOOKEEPER_ZNODE_PARENT));
-  hTable = new HTable(hbaseConf, hbaseTableName);
+  // Add vector for any column families not mentioned yet (in order to 
avoid
+  // creation of dummy NullableIntVectors for them).
+  for (HColumnDescriptor columnFamily :
--- End diff --

Created DRILL-4010 and added unit test (currently ignored) for 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43796603
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java
 ---
@@ -325,10 +325,13 @@ public AggOutcome doWork() {
 if (EXTRA_DEBUG_1) {
   logger.debug("Received new schema.  Batch has {} 
records.", incoming.getRecordCount());
 }
-newSchema = true;
-this.cleanup();
-// TODO: new schema case needs to be handled appropriately
-return AggOutcome.UPDATE_AGGREGATOR;
+final BatchSchema newIncomingSchema = incoming.getSchema();
+if ((! newIncomingSchema.equals(schema)) && schema != 
null) {
--- End diff --

> You shouldn't put a hack in to ignore a valid schema change.

Yeah, I'm reverting this HashAggTemplate change.

>  I don't know all the details but if this is related to HBase, my thought 
is you probably need
>  to resolve 4010 to avoid spurious schema changes. 

It's not clear whether it's related to HBase or not.  (The relevant test 
failure did occur in an HBase test, but it was in a Jenkins-run cluster test 
that has been hard to reproduce elsewhere.  At one point (I thought) I had a 
reproduction using just JSON files, but now that doesn't seem to work.)

> You should make sure to avoid propagating schema changes that are not 
real/required. 

Related to that, ExternalSortBatch seems to return two OK_NEW_SCHEMA 
batches with schemas differing only in their selection vectors: 

11:23:50.149 [main] TRACE o.a.d.e.p.i.v.IteratorValidatorBatchIterator - 
[#3; on ExternalSortBatch]: incoming next() return: #records = 204, 
  schema:
BatchSchema [fields=[`key1`(INT:REQUIRED), `alt`(BIGINT:REQUIRED)], 
selectionVector=FOUR_BYTE], 
  prev. new (not equal):
BatchSchema [fields=[`key1`(INT:REQUIRED), `alt`(BIGINT:REQUIRED)], 
selectionVector=NONE]

(The earlier schema ("prev. new:") seems to come from setting up the sort, 
and the latter one ("schema:") seems to come from completing the sort (it was 
triggered by a NONE from upstream).)

Is that correct?  (Should the selection vectors have been the same?  Should 
the second return have been OK instead of OK_NEW_SCHEMA?) 

If that is correct (e.g., vectors were changed and should have been), how 
should downstream operators, or at least  aggregation, handle that?

Should they be able to handle a "technical" schema change (OK_NEW_SCHEMA 
with possibly-changed vectors or certain details of the schema) that is not a 
logical schema change (not a change in the logical fields or types in the 
aggregated value), even if arbitrary logical schema changes can't be handled 
(e.g., because some don't even make sense)?




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43797898
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java
 ---
@@ -309,7 +309,14 @@ public Object getObject(int index) {
   Map vv = new JsonStringHashMap<>();
   for (String child:getChildFieldNames()) {
 ValueVector v = getChild(child);
-if (v != null) {
+// TODO(DRILL-4001):  Resolve this hack:
--- End diff --

Added mention of MapVector.getObject(int) and exception message 
"java.lang.IndexOutOfBoundsException: index: 0, length: 1 (expected: range(0, 
0))" to DRILL-4001 report.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43808779
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
 ---
@@ -93,13 +93,15 @@ public void ensureAtLeastOneField(ComplexWriter writer) 
{
 if (!atLeastOneWrite) {
   // if we had no columns, create one empty one so we can return some 
data for count purposes.
   SchemaPath sp = columns.get(0);
-  PathSegment root = sp.getRootSegment();
+  PathSegment fieldPath = sp.getRootSegment();
   BaseWriter.MapWriter fieldWriter = writer.rootAsMap();
-  while (root.getChild() != null && !root.getChild().isArray()) {
-fieldWriter = fieldWriter.map(root.getNameSegment().getPath());
-root = root.getChild();
+  while (fieldPath.getChild() != null && ! 
fieldPath.getChild().isArray()) {
+fieldWriter = 
fieldWriter.map(fieldPath.getNameSegment().getPath());
+fieldPath = fieldPath.getChild();
+  }
+  if (fieldWriter.isEmptyMap()) {
--- End diff --

Recall that ensureAtLeastOneField used to create that dummy 
NullableIntVector field/column (so that the schema didn't end up with zero 
fields/columns) _regardless_ of whether it needed to (i.e., even if the schema 
already had a column or columns from a previous JsonRecordReader for the same 
ScanBatch).  

Under the failure conditions, that blind vector setting frequently replaced 
a NullableVarCharVector with that NullableIntVector, causing either explicit 
"schema change not supported" user-targeted errors or internal errors about the 
mismatch between the NullableIntVector and the expectation of a 
NullableVarCharVector.

The additional check prevents ensureAtLeastOneField from overriding a 
column in an existing non-empty schema and causing those schema changes.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43921155
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java
 ---
@@ -325,10 +325,13 @@ public AggOutcome doWork() {
 if (EXTRA_DEBUG_1) {
   logger.debug("Received new schema.  Batch has {} 
records.", incoming.getRecordCount());
 }
-newSchema = true;
-this.cleanup();
-// TODO: new schema case needs to be handled appropriately
-return AggOutcome.UPDATE_AGGREGATOR;
+final BatchSchema newIncomingSchema = incoming.getSchema();
+if ((! newIncomingSchema.equals(schema)) && schema != 
null) {
--- End diff --

Reverted via a13e99b ("2288: Pt. 11 Core Upd.: REVERTED all of bad 
OK_NEW_SCHEMA schema comp…")


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/228#issuecomment-153831379
  
Note:  Something went wrong between me and Git.  I intended to address 
review comments only by adding commits (e.g., rather than by, say, deleting 
any), in order to avoid obscuring earlier comments here in the GitHub 
conversation.  However, for some reason, more than just the last four update 
commits (with "Upd.") show up as being added today.  I apparently re-ordered 
some commits during rebasing.





---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43941329
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
 ---
@@ -93,13 +93,15 @@ public void ensureAtLeastOneField(ComplexWriter writer) 
{
 if (!atLeastOneWrite) {
   // if we had no columns, create one empty one so we can return some 
data for count purposes.
   SchemaPath sp = columns.get(0);
-  PathSegment root = sp.getRootSegment();
+  PathSegment fieldPath = sp.getRootSegment();
   BaseWriter.MapWriter fieldWriter = writer.rootAsMap();
-  while (root.getChild() != null && !root.getChild().isArray()) {
-fieldWriter = fieldWriter.map(root.getNameSegment().getPath());
-root = root.getChild();
+  while (fieldPath.getChild() != null && ! 
fieldPath.getChild().isArray()) {
+fieldWriter = 
fieldWriter.map(fieldPath.getNameSegment().getPath());
+fieldPath = fieldPath.getChild();
+  }
+  if (fieldWriter.isEmptyMap()) {
--- End diff --

Sorry; I can't yet tell what you mean.

(By "the ensureAtLeastOneField operations," did you mean operations _in_ 
ensureAtLeastOneField (e.g., calling map(...) (in the loop)), the operation of 
calling ensureAtLeastOneField, or something else? (The first seems to be needed 
to get to the right map, the second seems to be needed to create a dummy column 
when one is actually needed.))







---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43943789
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
 ---
@@ -93,13 +93,15 @@ public void ensureAtLeastOneField(ComplexWriter writer) 
{
 if (!atLeastOneWrite) {
   // if we had no columns, create one empty one so we can return some 
data for count purposes.
   SchemaPath sp = columns.get(0);
-  PathSegment root = sp.getRootSegment();
+  PathSegment fieldPath = sp.getRootSegment();
   BaseWriter.MapWriter fieldWriter = writer.rootAsMap();
-  while (root.getChild() != null && !root.getChild().isArray()) {
-fieldWriter = fieldWriter.map(root.getNameSegment().getPath());
-root = root.getChild();
+  while (fieldPath.getChild() != null && ! 
fieldPath.getChild().isArray()) {
+fieldWriter = 
fieldWriter.map(fieldPath.getNameSegment().getPath());
+fieldPath = fieldPath.getChild();
+  }
+  if (fieldWriter.isEmptyMap()) {
--- End diff --

> can we remove the ensureAtLeastOneField property  

Oh, you mean the atLeastOneWrite property?

Yes, I think that will work.  I'll test that..


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on a diff in the pull request:

https://github.com/apache/drill/pull/228#discussion_r43946374
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
 ---
@@ -93,13 +93,15 @@ public void ensureAtLeastOneField(ComplexWriter writer) 
{
 if (!atLeastOneWrite) {
   // if we had no columns, create one empty one so we can return some 
data for count purposes.
   SchemaPath sp = columns.get(0);
-  PathSegment root = sp.getRootSegment();
+  PathSegment fieldPath = sp.getRootSegment();
   BaseWriter.MapWriter fieldWriter = writer.rootAsMap();
-  while (root.getChild() != null && !root.getChild().isArray()) {
-fieldWriter = fieldWriter.map(root.getNameSegment().getPath());
-root = root.getChild();
+  while (fieldPath.getChild() != null && ! 
fieldPath.getChild().isArray()) {
+fieldWriter = 
fieldWriter.map(fieldPath.getNameSegment().getPath());
+fieldPath = fieldPath.getChild();
+  }
+  if (fieldWriter.isEmptyMap()) {
--- End diff --

Another question:

> Basically change:
> 
> if (!atLeastOneWrite) {
> 
> to
> 
> if (fieldWriter.isEmptyMap()) {

Would that field writer be the writer from writer.rootAsMap(), or does the 
code still need the loop and its traversal?

(It seems that changing the "if (!atLeastOneWrite)" to (the simplified 
equivalent) of "if (true)" (and keeping the loop) work work, but I don't know 
what the minimum fix is (i.e., exactly how much of that loop and traversal is 
needed).)



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/228#issuecomment-153918109
  
Rebased on today's master branch.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2489 and DRILL-2769: JDBC exceptions (& ...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/171#issuecomment-154157257
  
Rebased on  current master.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/228#issuecomment-154281600
  
Addressed problems revealed by today's rebasing.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

GitHub user dsbos opened a pull request:

https://github.com/apache/drill/pull/245

DRILL-2288: Fix ScanBatch violation of IterOutcome protocol and downstream 
chain of bugs


Increments:

2288:  Pt. 1 Core:  Added unit test.  
[Drill2288GetColumnsMetadataWhenNoRowsTest, empty.json]

2288:  Pt. 1 Core:  Changed HBase test table #1's # of regions from 1 to 2. 
 [HBaseTestsSuite]

Also added TODO(DRILL-3954) comment about # of regions.

2288:  Pt. 2 Core:  Documented IterOutcome much more clearly.  [RecordBatch]

Also edited some related Javadoc.

2288:  Pt. 2 Hyg.:  Edited doc., added @Override, etc.  
[AbstractRecordBatch, RecordBatch]

Purged unused SetupOutcome.
Added @Override.
Edited comments.
Fix some comments to doc. comments.

2288:  Pt. 3 Core&Hyg.:  Added validation of IterOutcome sequence.  
[IteratorValidatorBatchIterator]

Also:
Renamed internal members for clarity.
Added comments.

2288:  Pt. 4 Core:  Fixed a NONE -> OK_NEW_SCHEMA in ScanBatch.next().  
[ScanBatch]

(With nearby comments.)

2288:  Pt. 4 Hyg.:  Edited comments, reordered, whitespace.  [ScanBatch]

Reordered
Added comments.
Aligned.

2288:  Pt. 4 Core+:  Fixed UnionAllRecordBatch to receive IterOutcome 
sequence right.  (3659)  [UnionAllRecordBatch]

2288:  Pt. 5 Core:  Fixed ScanBatch.Mutator.isNewSchema() to stop spurious 
"new schema" reports (fix short-circuit OR, to call resetting method right).  
[ScanBatch]

2288:  Pt. 5 Hyg.:  Renamed, edited comments, reordered.  [ScanBatch, 
SchemaChangeCallBack, AbstractSingleRecordBatch]

Renamed getSchemaChange -> getSchemaChangedAndReset.
Renamed schemaChange -> schemaChanged.
Added doc. comments.
Aligned.

2288:  Pt. 6 Core:  Avoided dummy Null.IntVec. column in JsonReader when 
not needed (MapWriter.isEmptyMap()).  [JsonReader, 3 vector files]

2288:  Pt. 6 Hyg.:  Edited comments, message.  Fixed message formatting.  
[RecordReader, JSONFormatPlugin, JSONRecordReader, AbstractMapVector, 
JsonReader]

Fixed message formatting.
Edited comments.
Edited message.
Fixed spurious line break.

2288:  Pt. 7 Core:  Added column families in HBaseRecordReader* to avoid 
dummy Null.IntVec. clash.  [HBaseRecordReader]

2288:  Pt. 8 Core.1:  Cleared recordCount in 
OrderedPartitionRecordBatch.innerNext().  [OrderedPartitionRecordBatch]

2288:  Pt. 8 Core.2:  Cleared recordCount in ProjectRecordBatch.innerNext.  
[ProjectRecordBatch]

2288:  Pt. 8 Core.3:  Cleared recordCount in TopNBatch.innerNext.  
[TopNBatch]

2288:  Pt. 9 Core:  Had UnorderedReceiverBatch reset RecordBatchLoader's 
record count.  [UnorderedReceiverBatch, RecordBatchLoader]

2288:  Pt. 9 Hyg.:  Added comments.  [RecordBatchLoader]

2288:  Pt. 10 Core:  Worked around mismatched map child vectors in 
MapVector.getObject().  [MapVector]

2288:  Pt. 11 Core:  Added OK_NEW_SCHEMA schema comparison for HashAgg.  
[HashAggTemplate]

2288:  Pt. 12 Core:  Fixed memory leak in BaseTestQuery's printing.

Fixed bad skipping of RecordBatchLoader.clear(...) and
QueryDataBatch.load(...) for zero-row batches in printResult(...).

Also, dropped suppression of call to
VectorUtil.showVectorAccessibleContent(...) (so zero-row batches are
as visible as others).

2288:  Pt. 13 Core:  Fixed test that used unhandled periods in column alias 
identifiers.

2288:  Misc.:  Added # of rows to showVectorAccessibleContent's output.  
[VectorUtil]

2288:  Misc.:  Added simple/partial toString() [VectorContainer, 
AbstractRecordReader, JSONRecordReader, BaseValueVector, FieldSelection, 
AbstractBaseWriter]

2288:  Misc. Hyg.:  Added doc. comments to VectorContainer.  
[VectorContainer]

2288:  Misc. Hyg.:  Edited comment.  [DrillStringUtils]

2288:  Misc. Hyg.:  Clarified message for unhandled identifier containing 
period.

2288:  Pt. 3 Core&Hyg. Upd.:  Added schema comparison result to logging.  
[IteratorValidatorBatchIterator]

2288:  Pt. 7 Core Upd.:  Handled HBase columns too re NullableIntVectors.  
[HBaseRecordReader, TestTableGenerator, TestHBaseFilterPushDown]

Created map-child vectors for requested columns.
Added unit test method testDummyColumnsAreAvoided, adding new row to test 
table,
updated some row counts.

2288:  Pt. 7 Hyg. Upd.:  Edited comment.  [HBaseRecordReader]

2288:  Pt. 11 Core Upd.:  REVERTED all of bad OK_NEW_SCHEMA schema 
comparison for HashAgg.  [HashAggTemplate]

This reverts commit 0939660f4620c03da97f4e1bf25a27514e6d0b81.

2288:  Pt. 6 Core Upd.:  Added isEmptyMap override in new (just-rebased-in) 
PromotableWriter.  [PromotableW

[GitHub] drill pull request: DRILL-3848: Increase timeout time on several t...

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/174


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3641: Doc. RecordBatch.IterOutcome (enum...

[dsbos]

Github user dsbos commented on the pull request:

https://github.com/apache/drill/pull/113#issuecomment-166020356
  
Resolved as part of DRILL-2288 patch.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-3641: Doc. RecordBatch.IterOutcome (enum...

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/113


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/245


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2288: Fix ScanBatch violation of IterOut...

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/228


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request: DRILL-2489 and DRILL-2769: JDBC exceptions (& ...

[dsbos]

Github user dsbos closed the pull request at:

https://github.com/apache/drill/pull/171


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---