You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by hnfgns <gi...@git.apache.org> on 2015/11/19 22:20:47 UTC
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
GitHub user hnfgns opened a pull request:
https://github.com/apache/drill/pull/270
DRILL-2618: handle queries over empty folders consistently so that th…
…ey report table not found rather than failing.
Refactor FileSelection to eliminate redundancy, make it more managable
Fix WorkspaceSchemaFactory to handle empty folders.
Introduce ParquetFileSelection, a sub-class of FileSelection that carries along metadata cache
Fix MagicStringMatcher so that it operate on files only.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hnfgns/incubator-drill DRILL-2618
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/270.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 #270
----
commit 51de65851c5d355d88f682bc0fc9c0853f216d3c
Author: Hanifi Gunes <ha...@gmail.com>
Date: 2015-11-17T02:33:13Z
DRILL-2618: handle queries over empty folders consistently so that they report table not found rather than failing.
Refactor FileSelection to eliminate redundancy, make it more managable
Fix WorkspaceSchemaFactory to handle empty folders.
Introduce ParquetFileSelection, a sub-class of FileSelection that carries along metadata cache
Fix MagicStringMatcher so that it operate on files only.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45417976
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -102,77 +92,33 @@ public boolean containsDirectories(DrillFileSystem fs) throws IOException {
}
public FileSelection minusDirectories(DrillFileSystem fs) throws IOException {
- Stopwatch timer = new Stopwatch();
- timer.start();
- init(fs);
- List<FileStatus> newList = Lists.newArrayList();
- for (FileStatus p : statuses) {
- if (p.isDirectory()) {
- List<FileStatus> statuses = fs.list(true, p.getPath());
- for (FileStatus s : statuses) {
- newList.add(s);
- }
- } else {
- newList.add(p);
- }
- }
- logger.info("FileSelection.minusDirectories() took {} ms, numFiles: {}",
- timer.elapsed(TimeUnit.MILLISECONDS), newList.size());
- return new FileSelection(newList, selectionRoot);
- }
-
- public FileStatus getFirstPath(DrillFileSystem fs) throws IOException {
- init(fs);
- return statuses.get(0);
- }
-
- public List<String> getAsFiles() {
- if (!files.isEmpty()) {
- return files;
- }
- if (statuses == null) {
- return Collections.emptyList();
+ final List<FileStatus> statuses = getStatuses(fs);
+ final int total = statuses.size();
+ final Path[] paths = new Path[total];
+ for (int i=0; i<total; i++) {
+ paths[i] = statuses.get(i).getPath();
}
- List<String> files = Lists.newArrayList();
- for (FileStatus s : statuses) {
- files.add(s.getPath().toString());
- }
- return files;
- }
-
- private void init(DrillFileSystem fs) throws IOException {
- Stopwatch timer = new Stopwatch();
- timer.start();
- if (files != null && statuses == null) {
- statuses = Lists.newArrayList();
- for (String p : files) {
- statuses.add(fs.getFileStatus(new Path(p)));
+ final List<FileStatus> allStats = fs.list(true, paths);
+ final List<FileStatus> nonDirectories = Lists.newArrayList(Iterables.filter(allStats, new Predicate<FileStatus>() {
+ @Override
+ public boolean apply(@Nullable FileStatus status) {
+ return !status.isDirectory();
}
- }
- logger.info("FileSelection.init() took {} ms, numFiles: {}",
- timer.elapsed(TimeUnit.MILLISECONDS), statuses == null ? 0 : statuses.size());
- }
+ }));
- public List<FileStatus> getFileStatusList(DrillFileSystem fs) throws IOException {
- init(fs);
- return statuses;
+ return create(nonDirectories, null, selectionRoot);
}
- /**
- * Return the parquet table metadata that may have been read
- * from a metadata cache file during creation of this file selection.
- * It will always be null for non-parquet files and null for cases
- * where no metadata cache was created.
- */
- public ParquetTableMetadata_v1 getParquetMetadata() {
- return parquetMeta;
+ public FileStatus getFirstPath(DrillFileSystem fs) throws IOException {
+ return getStatuses(fs).get(0);
}
- private static String commonPath(FileStatus... paths) {
+ private static String commonPath(List<FileStatus> statuses) {
--- End diff --
this method assumes statuses is not null. This is not always the case, see my comment below
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45417859
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -190,22 +136,40 @@ private static String commonPath(FileStatus... paths) {
break;
}
}
- URI oneURI = paths[0].getPath().toUri();
+ URI oneURI = statuses.get(0).getPath().toUri();
return new Path(oneURI.getScheme(), oneURI.getAuthority(), commonPath).toString();
}
- public static FileSelection create(DrillFileSystem fs, String parent, String path) throws IOException {
- Path p = new Path(parent,removeLeadingSlash(path));
- FileStatus[] status = fs.globStatus(p);
- if (status == null || status.length == 0) {
+ public static FileSelection create(final DrillFileSystem fs, final String parent, final String path) throws IOException {
+ final Path combined = new Path(parent, removeLeadingSlash(path));
+ final FileStatus[] statuses = fs.globStatus(combined);
+ if (statuses == null) {
return null;
}
- if (status.length == 1) {
--- End diff --
I don't understand the change here. I will leave this for @amansinha100
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45509247
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -190,22 +136,40 @@ private static String commonPath(FileStatus... paths) {
break;
}
}
- URI oneURI = paths[0].getPath().toUri();
+ URI oneURI = statuses.get(0).getPath().toUri();
return new Path(oneURI.getScheme(), oneURI.getAuthority(), commonPath).toString();
}
- public static FileSelection create(DrillFileSystem fs, String parent, String path) throws IOException {
- Path p = new Path(parent,removeLeadingSlash(path));
- FileStatus[] status = fs.globStatus(p);
- if (status == null || status.length == 0) {
+ public static FileSelection create(final DrillFileSystem fs, final String parent, final String path) throws IOException {
+ final Path combined = new Path(parent, removeLeadingSlash(path));
+ final FileStatus[] statuses = fs.globStatus(combined);
+ if (statuses == null) {
return null;
}
- if (status.length == 1) {
- URI oneURI = status[0].getPath().toUri();
- String selectionRoot = new Path(oneURI.getScheme(), oneURI.getAuthority(), p.toUri().getPath()).toString();
- return new FileSelection(Collections.singletonList(status[0]), selectionRoot);
+ return create(Lists.newArrayList(statuses), null, combined.toUri().toString());
+ }
+
+ /**
+ * Creates a {@link FileSelection selection} with the given file statuses/files and selection root.
+ *
+ * @param statuses list of file statuses
+ * @param files list of files
+ * @param root root path for selections
+ *
+ * @return null if creation of {@link FileSelection} fails with an {@link IllegalArgumentException}
+ * otherwise a new selection.
+ *
+ * @see FileSelection#FileSelection(List, List, String)
+ */
+ public static FileSelection create(final List<FileStatus> statuses, final List<String> files, final String root) {
+ try {
+ final String selectionRoot = Strings.isNullOrEmpty(root) ? commonPath(statuses) : root;
+ return new FileSelection(statuses, files, selectionRoot);
+ } catch (IllegalArgumentException ex) {
+ // we defer task of validating arguments for code consistency and inheritance support.
+ logger.debug("unable to create FileSelection", ex);
+ return null;
--- End diff --
I will create a separate JIRA for investigating all cases of null outcome handling.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45413002
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -19,82 +19,72 @@
import java.io.IOException;
import java.net.URI;
-import java.util.Collections;
import java.util.List;
-import java.util.concurrent.TimeUnit;
-
-import com.google.common.base.Stopwatch;
-import org.apache.commons.lang3.ArrayUtils;
-import org.apache.commons.lang3.StringUtils;
-import org.apache.drill.exec.store.parquet.Metadata.ParquetTableMetadata_v1;
-import org.apache.hadoop.fs.FileStatus;
-import org.apache.hadoop.fs.Path;
+import javax.annotation.Nullable;
import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Predicate;
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
/**
- * Jackson serializable description of a file selection. Maintains an internal set of file statuses. However, also
- * serializes out as a list of Strings. All accessing methods first regenerate the FileStatus objects if they are not
- * available. This allows internal movement of FileStatus and the ability to serialize if need be.
+ * Jackson serializable description of a file selection.
*/
public class FileSelection {
static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FileSelection.class);
@JsonIgnore
- private List<FileStatus> statuses;
+ public List<FileStatus> statuses;
public List<String> files;
public String selectionRoot;
- // this is a temporary location for the reference to Parquet metadata
- // TODO: ideally this should be in a Parquet specific derived class.
- private ParquetTableMetadata_v1 parquetMeta = null;
-
- public FileSelection() {
- }
-
- public FileSelection(List<String> files, String selectionRoot, boolean dummy) {
- this.files = files;
- this.selectionRoot = selectionRoot;
- }
-
- public FileSelection(List<String> files, boolean dummy) {
- this.files = files;
- }
-
- public FileSelection(List<FileStatus> statuses) {
- this(statuses, null);
- }
-
- public FileSelection(List<String> files, String selectionRoot,
- ParquetTableMetadata_v1 meta) {
+ /**
+ * Creates a {@link FileSelection selection} out of given file statuses/files and selection root.
+ *
+ * @param statuses list of file statuses
+ * @param files list of files
+ * @param selectionRoot root path for selections
+ *
+ * @throws IllegalArgumentException if the list of statuses and files are null or empty.
+ */
+ protected FileSelection(List<FileStatus> statuses, List<String> files, String selectionRoot) {
+ final boolean emptySelection = (statuses == null || statuses.size() == 0) && (files == null || files.size() == 0);
+ Preconditions.checkArgument(!emptySelection, "Either statuses or files must be supplied not both/node of them");
--- End diff --
small typo here "node" should read "none"
also this doesn't actually check if both of them are not null, let the message seem to suggest
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45427577
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -19,82 +19,72 @@
import java.io.IOException;
import java.net.URI;
-import java.util.Collections;
import java.util.List;
-import java.util.concurrent.TimeUnit;
-
-import com.google.common.base.Stopwatch;
-import org.apache.commons.lang3.ArrayUtils;
-import org.apache.commons.lang3.StringUtils;
-import org.apache.drill.exec.store.parquet.Metadata.ParquetTableMetadata_v1;
-import org.apache.hadoop.fs.FileStatus;
-import org.apache.hadoop.fs.Path;
+import javax.annotation.Nullable;
import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Predicate;
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
/**
- * Jackson serializable description of a file selection. Maintains an internal set of file statuses. However, also
- * serializes out as a list of Strings. All accessing methods first regenerate the FileStatus objects if they are not
- * available. This allows internal movement of FileStatus and the ability to serialize if need be.
+ * Jackson serializable description of a file selection.
*/
public class FileSelection {
static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FileSelection.class);
@JsonIgnore
- private List<FileStatus> statuses;
+ public List<FileStatus> statuses;
public List<String> files;
public String selectionRoot;
- // this is a temporary location for the reference to Parquet metadata
- // TODO: ideally this should be in a Parquet specific derived class.
- private ParquetTableMetadata_v1 parquetMeta = null;
-
- public FileSelection() {
- }
-
- public FileSelection(List<String> files, String selectionRoot, boolean dummy) {
- this.files = files;
- this.selectionRoot = selectionRoot;
- }
-
- public FileSelection(List<String> files, boolean dummy) {
- this.files = files;
- }
-
- public FileSelection(List<FileStatus> statuses) {
- this(statuses, null);
- }
-
- public FileSelection(List<String> files, String selectionRoot,
- ParquetTableMetadata_v1 meta) {
+ /**
+ * Creates a {@link FileSelection selection} out of given file statuses/files and selection root.
+ *
+ * @param statuses list of file statuses
+ * @param files list of files
+ * @param selectionRoot root path for selections
+ *
+ * @throws IllegalArgumentException if the list of statuses and files are null or empty.
+ */
+ protected FileSelection(List<FileStatus> statuses, List<String> files, String selectionRoot) {
+ final boolean emptySelection = (statuses == null || statuses.size() == 0) && (files == null || files.size() == 0);
+ Preconditions.checkArgument(!emptySelection, "Either statuses or files must be supplied not both/node of them");
--- 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45417180
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -190,22 +136,40 @@ private static String commonPath(FileStatus... paths) {
break;
}
}
- URI oneURI = paths[0].getPath().toUri();
+ URI oneURI = statuses.get(0).getPath().toUri();
return new Path(oneURI.getScheme(), oneURI.getAuthority(), commonPath).toString();
}
- public static FileSelection create(DrillFileSystem fs, String parent, String path) throws IOException {
- Path p = new Path(parent,removeLeadingSlash(path));
- FileStatus[] status = fs.globStatus(p);
- if (status == null || status.length == 0) {
+ public static FileSelection create(final DrillFileSystem fs, final String parent, final String path) throws IOException {
+ final Path combined = new Path(parent, removeLeadingSlash(path));
+ final FileStatus[] statuses = fs.globStatus(combined);
+ if (statuses == null) {
--- End diff --
is it possible to have `statuses.length == 0` ? in this case the call to `create()` below will fail
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45427442
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -19,82 +19,72 @@
import java.io.IOException;
import java.net.URI;
-import java.util.Collections;
import java.util.List;
-import java.util.concurrent.TimeUnit;
-
-import com.google.common.base.Stopwatch;
-import org.apache.commons.lang3.ArrayUtils;
-import org.apache.commons.lang3.StringUtils;
-import org.apache.drill.exec.store.parquet.Metadata.ParquetTableMetadata_v1;
-import org.apache.hadoop.fs.FileStatus;
-import org.apache.hadoop.fs.Path;
+import javax.annotation.Nullable;
import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Predicate;
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
/**
- * Jackson serializable description of a file selection. Maintains an internal set of file statuses. However, also
- * serializes out as a list of Strings. All accessing methods first regenerate the FileStatus objects if they are not
- * available. This allows internal movement of FileStatus and the ability to serialize if need be.
+ * Jackson serializable description of a file selection.
*/
public class FileSelection {
static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FileSelection.class);
@JsonIgnore
- private List<FileStatus> statuses;
+ public List<FileStatus> statuses;
--- End diff --
good point. 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45432185
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -102,77 +92,33 @@ public boolean containsDirectories(DrillFileSystem fs) throws IOException {
}
public FileSelection minusDirectories(DrillFileSystem fs) throws IOException {
- Stopwatch timer = new Stopwatch();
- timer.start();
- init(fs);
- List<FileStatus> newList = Lists.newArrayList();
- for (FileStatus p : statuses) {
- if (p.isDirectory()) {
- List<FileStatus> statuses = fs.list(true, p.getPath());
- for (FileStatus s : statuses) {
- newList.add(s);
- }
- } else {
- newList.add(p);
- }
- }
- logger.info("FileSelection.minusDirectories() took {} ms, numFiles: {}",
--- End diff --
@amansinha100 do we still need this log information ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45432737
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -190,22 +136,40 @@ private static String commonPath(FileStatus... paths) {
break;
}
}
- URI oneURI = paths[0].getPath().toUri();
+ URI oneURI = statuses.get(0).getPath().toUri();
return new Path(oneURI.getScheme(), oneURI.getAuthority(), commonPath).toString();
}
- public static FileSelection create(DrillFileSystem fs, String parent, String path) throws IOException {
- Path p = new Path(parent,removeLeadingSlash(path));
- FileStatus[] status = fs.globStatus(p);
- if (status == null || status.length == 0) {
+ public static FileSelection create(final DrillFileSystem fs, final String parent, final String path) throws IOException {
+ final Path combined = new Path(parent, removeLeadingSlash(path));
+ final FileStatus[] statuses = fs.globStatus(combined);
+ if (statuses == null) {
--- End diff --
if statuses is empty (but not null), we will call create below passing an empty statuses list and a null files list which will cause the precondition of the method to fail, right ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45420041
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetFileSelection.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.parquet;
+
+import com.google.common.base.Preconditions;
+import org.apache.drill.exec.store.dfs.FileSelection;
+import org.apache.drill.exec.store.parquet.Metadata.ParquetTableMetadata_v1;
+
+/**
+ * Parquet specific {@link FileSelection selection} that carries out {@link ParquetTableMetadata_v1 metadata} along.
+ */
+public class ParquetFileSelection extends FileSelection {
+ static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(ParquetFileSelection.class);
--- End diff --
can you make this private, too ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/270#issuecomment-158266801
+1 overall, there are 2 places where @amansinha100 could give his opinion about
can we add a unit test too ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45418939
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -190,22 +136,40 @@ private static String commonPath(FileStatus... paths) {
break;
}
}
- URI oneURI = paths[0].getPath().toUri();
+ URI oneURI = statuses.get(0).getPath().toUri();
return new Path(oneURI.getScheme(), oneURI.getAuthority(), commonPath).toString();
}
- public static FileSelection create(DrillFileSystem fs, String parent, String path) throws IOException {
- Path p = new Path(parent,removeLeadingSlash(path));
- FileStatus[] status = fs.globStatus(p);
- if (status == null || status.length == 0) {
+ public static FileSelection create(final DrillFileSystem fs, final String parent, final String path) throws IOException {
+ final Path combined = new Path(parent, removeLeadingSlash(path));
+ final FileStatus[] statuses = fs.globStatus(combined);
+ if (statuses == null) {
return null;
}
- if (status.length == 1) {
- URI oneURI = status[0].getPath().toUri();
- String selectionRoot = new Path(oneURI.getScheme(), oneURI.getAuthority(), p.toUri().getPath()).toString();
- return new FileSelection(Collections.singletonList(status[0]), selectionRoot);
+ return create(Lists.newArrayList(statuses), null, combined.toUri().toString());
+ }
+
+ /**
+ * Creates a {@link FileSelection selection} with the given file statuses/files and selection root.
+ *
+ * @param statuses list of file statuses
+ * @param files list of files
+ * @param root root path for selections
+ *
+ * @return null if creation of {@link FileSelection} fails with an {@link IllegalArgumentException}
+ * otherwise a new selection.
+ *
+ * @see FileSelection#FileSelection(List, List, String)
+ */
+ public static FileSelection create(final List<FileStatus> statuses, final List<String> files, final String root) {
+ try {
+ final String selectionRoot = Strings.isNullOrEmpty(root) ? commonPath(statuses) : root;
+ return new FileSelection(statuses, files, selectionRoot);
+ } catch (IllegalArgumentException ex) {
+ // we defer task of validating arguments for code consistency and inheritance support.
+ logger.debug("unable to create FileSelection", ex);
+ return null;
--- End diff --
not all code paths seem to be checking if this returns null. For example `FileSystemPartitionDescriptor.createNewGroupScan()` passes the output to `FileGroupScan.close()` which expects it to be not null
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45509104
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -190,22 +136,40 @@ private static String commonPath(FileStatus... paths) {
break;
}
}
- URI oneURI = paths[0].getPath().toUri();
+ URI oneURI = statuses.get(0).getPath().toUri();
return new Path(oneURI.getScheme(), oneURI.getAuthority(), commonPath).toString();
}
- public static FileSelection create(DrillFileSystem fs, String parent, String path) throws IOException {
- Path p = new Path(parent,removeLeadingSlash(path));
- FileStatus[] status = fs.globStatus(p);
- if (status == null || status.length == 0) {
+ public static FileSelection create(final DrillFileSystem fs, final String parent, final String path) throws IOException {
+ final Path combined = new Path(parent, removeLeadingSlash(path));
+ final FileStatus[] statuses = fs.globStatus(combined);
+ if (statuses == null) {
return null;
}
- if (status.length == 1) {
- URI oneURI = status[0].getPath().toUri();
- String selectionRoot = new Path(oneURI.getScheme(), oneURI.getAuthority(), p.toUri().getPath()).toString();
- return new FileSelection(Collections.singletonList(status[0]), selectionRoot);
+ return create(Lists.newArrayList(statuses), null, combined.toUri().toString());
+ }
+
+ /**
+ * Creates a {@link FileSelection selection} with the given file statuses/files and selection root.
+ *
+ * @param statuses list of file statuses
+ * @param files list of files
+ * @param root root path for selections
+ *
+ * @return null if creation of {@link FileSelection} fails with an {@link IllegalArgumentException}
+ * otherwise a new selection.
+ *
+ * @see FileSelection#FileSelection(List, List, String)
+ */
+ public static FileSelection create(final List<FileStatus> statuses, final List<String> files, final String root) {
+ try {
+ final String selectionRoot = Strings.isNullOrEmpty(root) ? commonPath(statuses) : root;
--- End diff --
thanks. 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45419992
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -19,82 +19,72 @@
import java.io.IOException;
import java.net.URI;
-import java.util.Collections;
import java.util.List;
-import java.util.concurrent.TimeUnit;
-
-import com.google.common.base.Stopwatch;
-import org.apache.commons.lang3.ArrayUtils;
-import org.apache.commons.lang3.StringUtils;
-import org.apache.drill.exec.store.parquet.Metadata.ParquetTableMetadata_v1;
-import org.apache.hadoop.fs.FileStatus;
-import org.apache.hadoop.fs.Path;
+import javax.annotation.Nullable;
import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Predicate;
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
/**
- * Jackson serializable description of a file selection. Maintains an internal set of file statuses. However, also
- * serializes out as a list of Strings. All accessing methods first regenerate the FileStatus objects if they are not
- * available. This allows internal movement of FileStatus and the ability to serialize if need be.
+ * Jackson serializable description of a file selection.
*/
public class FileSelection {
static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FileSelection.class);
--- End diff --
can you make this private ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45427970
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -190,22 +136,40 @@ private static String commonPath(FileStatus... paths) {
break;
}
}
- URI oneURI = paths[0].getPath().toUri();
+ URI oneURI = statuses.get(0).getPath().toUri();
return new Path(oneURI.getScheme(), oneURI.getAuthority(), commonPath).toString();
}
- public static FileSelection create(DrillFileSystem fs, String parent, String path) throws IOException {
- Path p = new Path(parent,removeLeadingSlash(path));
- FileStatus[] status = fs.globStatus(p);
- if (status == null || status.length == 0) {
+ public static FileSelection create(final DrillFileSystem fs, final String parent, final String path) throws IOException {
+ final Path combined = new Path(parent, removeLeadingSlash(path));
+ final FileStatus[] statuses = fs.globStatus(combined);
+ if (statuses == null) {
--- End diff --
We handle null case explicitly here as we cannot create a List down when statuses is null. In case statuses is empty, the delegate create returns `null` which is expected. Can you clarify what you mean by failure?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/270
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45509395
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetFileSelection.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.parquet;
+
+import com.google.common.base.Preconditions;
+import org.apache.drill.exec.store.dfs.FileSelection;
+import org.apache.drill.exec.store.parquet.Metadata.ParquetTableMetadata_v1;
+
+/**
+ * Parquet specific {@link FileSelection selection} that carries out {@link ParquetTableMetadata_v1 metadata} along.
+ */
+public class ParquetFileSelection extends FileSelection {
+ static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(ParquetFileSelection.class);
--- End diff --
sure. 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45412874
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -19,82 +19,72 @@
import java.io.IOException;
import java.net.URI;
-import java.util.Collections;
import java.util.List;
-import java.util.concurrent.TimeUnit;
-
-import com.google.common.base.Stopwatch;
-import org.apache.commons.lang3.ArrayUtils;
-import org.apache.commons.lang3.StringUtils;
-import org.apache.drill.exec.store.parquet.Metadata.ParquetTableMetadata_v1;
-import org.apache.hadoop.fs.FileStatus;
-import org.apache.hadoop.fs.Path;
+import javax.annotation.Nullable;
import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Predicate;
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
/**
- * Jackson serializable description of a file selection. Maintains an internal set of file statuses. However, also
- * serializes out as a list of Strings. All accessing methods first regenerate the FileStatus objects if they are not
- * available. This allows internal movement of FileStatus and the ability to serialize if need be.
+ * Jackson serializable description of a file selection.
*/
public class FileSelection {
static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FileSelection.class);
@JsonIgnore
- private List<FileStatus> statuses;
+ public List<FileStatus> statuses;
--- End diff --
Only `ParquetFileSelection` requires this to be public. Can we make this private and another constructor instead ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2618: handle queries over empty folders ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/270#discussion_r45418223
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java ---
@@ -190,22 +136,40 @@ private static String commonPath(FileStatus... paths) {
break;
}
}
- URI oneURI = paths[0].getPath().toUri();
+ URI oneURI = statuses.get(0).getPath().toUri();
return new Path(oneURI.getScheme(), oneURI.getAuthority(), commonPath).toString();
}
- public static FileSelection create(DrillFileSystem fs, String parent, String path) throws IOException {
- Path p = new Path(parent,removeLeadingSlash(path));
- FileStatus[] status = fs.globStatus(p);
- if (status == null || status.length == 0) {
+ public static FileSelection create(final DrillFileSystem fs, final String parent, final String path) throws IOException {
+ final Path combined = new Path(parent, removeLeadingSlash(path));
+ final FileStatus[] statuses = fs.globStatus(combined);
+ if (statuses == null) {
return null;
}
- if (status.length == 1) {
- URI oneURI = status[0].getPath().toUri();
- String selectionRoot = new Path(oneURI.getScheme(), oneURI.getAuthority(), p.toUri().getPath()).toString();
- return new FileSelection(Collections.singletonList(status[0]), selectionRoot);
+ return create(Lists.newArrayList(statuses), null, combined.toUri().toString());
+ }
+
+ /**
+ * Creates a {@link FileSelection selection} with the given file statuses/files and selection root.
+ *
+ * @param statuses list of file statuses
+ * @param files list of files
+ * @param root root path for selections
+ *
+ * @return null if creation of {@link FileSelection} fails with an {@link IllegalArgumentException}
+ * otherwise a new selection.
+ *
+ * @see FileSelection#FileSelection(List, List, String)
+ */
+ public static FileSelection create(final List<FileStatus> statuses, final List<String> files, final String root) {
+ try {
+ final String selectionRoot = Strings.isNullOrEmpty(root) ? commonPath(statuses) : root;
--- End diff --
`FormatSelection` C'tor calls this method passing `statuses` and `root` as null. this will call `commonPath()` passing null as a parameter, which will cause a NPE
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---