[GitHub] drill pull request #972: DRILL-5838: Fix MaprDB filter pushdown for the case...
Github user asfgit closed the pull request at: https://github.com/apache/drill/pull/972 ---
[GitHub] drill pull request #972: DRILL-5838: Fix MaprDB filter pushdown for the case...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/972#discussion_r143107921
--- Diff:
contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/json/JsonConditionBuilder.java
---
@@ -159,73 +159,73 @@ private void setIsCondition(QueryCondition c,
private JsonScanSpec createJsonScanSpec(FunctionCall call,
CompareFunctionsProcessor processor) {
String functionName = processor.getFunctionName();
-SchemaPath field = processor.getPath();
+String fieldPath = processor.getPath().asPathString(false);
--- End diff --
This needs an explanation. This is a `String`. The method is
`asPathString()` which looks like we will take name parts ["a", "b.c", "d"] to
produce "a.b.c.d". This cannot be right as either 1) it is represents a path to
be split, or 2) represents a full name to match. But, I might have fields
["a.b", "c.d"] which also expands to "a.b.c.d". So, the matching part can't be
right. The path split part can't be right since we cannot recover the name
parts from the string.
In short, should we path the parsed path (collection of name parts) to the
functions?
See comments for more info.
---
[GitHub] drill pull request #972: DRILL-5838: Fix MaprDB filter pushdown for the case...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/972#discussion_r143107072
--- Diff:
logical/src/main/java/org/apache/drill/common/expression/PathSegment.java ---
@@ -151,6 +151,15 @@ public boolean isNamed() {
return true;
}
+/**
+ * Checks that the path of this name segment is complex.
+ *
+ * @return true if the path of this name segment contains dots
+ */
+public boolean isComplex() {
--- End diff --
Not sure I agree with this. We just did changes to enforce the rule that a
path segment is represented by a distinct object. If a path segment can contain
dots, then we don't need to represent parts as multiple parts. But, we went the
multiple part route to allow dots in names.
How will we know if a dot in a name represents a complex (multi-part) name
vs. a simple name that happens to contain a dot?
In short, this seems a bit of a hack and introduces undesirable ambiguity.
---
[GitHub] drill pull request #972: DRILL-5838: Fix MaprDB filter pushdown for the case...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/972#discussion_r143107377
--- Diff:
logical/src/main/java/org/apache/drill/common/expression/SchemaPath.java ---
@@ -264,6 +264,56 @@ public String getRootSegmentPath() {
return rootSegment.getPath();
}
+ /**
+ * Returns {@code String} representation of this schema path,
+ * quoting all name segments if specified {@code quote} is true or
quoting
+ * only those name segments which have a complex name (their name
contains dots).
+ *
+ * @param quoted is name segment should be quoted
+ * @return the {@code String} representation of this {@code SchemaPath}
+ * @throws IllegalStateException if root segment is {@code ArraySegment}
+ */
+ public String asPathString(boolean quoted) throws RuntimeException {
+StringBuilder sb = new StringBuilder();
+PathSegment seg = rootSegment;
+if (seg.isArray()) {
+ throw new IllegalStateException("Drill doesn't currently support top
level arrays");
+}
+NameSegment nameSegment = seg.getNameSegment();
+writeQuoted(sb, nameSegment.getPath(), quoted ||
nameSegment.isComplex());
+
+while ((seg = seg.getChild()) != null) {
+ if (seg.isNamed()) {
+nameSegment = seg.getNameSegment();
+sb.append('.');
+writeQuoted(sb, nameSegment.getPath(), quoted ||
nameSegment.isComplex());
--- End diff --
If `isComplex()` is meant to indicate that a name must be quoted, then
`requiresQuotes()` would be a better name. Some other names that require quotes:
{noformat}
names with spaces
names-with-dashes
anything/with+an*operator
$looks$like$internal$name
maybeEvenCaseSensitive
MAYBEeVENcASEsENSIVIVE
!on
...
{noformat}
Basically, anything that is not a symbol (initial alpha followed by any
number of alphanumeric...)
---
[GitHub] drill pull request #972: DRILL-5838: Fix MaprDB filter pushdown for the case...
GitHub user vvysotskyi opened a pull request: https://github.com/apache/drill/pull/972 DRILL-5838: Fix MaprDB filter pushdown for the case of nested field (reg. of DRILL-4264) Please see [DRILL-4264|https://issues.apache.org/jira/browse/DRILL-5838] for the problem description. `FieldPath.asPathString()` returns string in which field names may be quoted if `FieldPath` instance was created from the string, in which these fields were quoted. `SchemaPath.toExpr()` returns string were all field names are quoted. - Created new method in `SchemaPath` which quotes only field names which contain dots. - Fixed existing MaprDB unit tests to handle this case. You can merge this pull request into a Git repository by running: $ git pull https://github.com/vvysotskyi/drill DRILL-5838 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/972.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 #972 ---
