[GitHub] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/incubator-hawq/pull/906


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[kavinderd]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83103736
  
--- Diff: 
pxf/pxf-hive/src/main/java/org/apache/hawq/pxf/plugins/hive/HiveDataFragmenter.java
 ---
@@ -404,24 +411,41 @@ private String buildFilterStringForHive() throws 
Exception {
 HiveFilterBuilder eval = new HiveFilterBuilder(inputData);
 Object filter = eval.getFilterObject(filterInput);
 
-String prefix = "";
-
-if (filter instanceof List) {
-
-for (Object f : (List) filter) {
-if (buildSingleFilter(f, filtersString, prefix)) {
-// Set 'and' operator between each matched partition 
filter.
-prefix = " and ";
-}
-}
-
+if (filter instanceof LogicalFilter) {
+buildCompoundFilter((LogicalFilter) filter, filtersString);
 } else {
-buildSingleFilter(filter, filtersString, prefix);
+buildSingleFilter(filter, filtersString, "");
 }
 
 return filtersString.toString();
 }
 
+private void buildCompoundFilter(LogicalFilter filter, StringBuilder 
filterString) throws Exception{
+String prefix;
+switch(filter.getOperator()) {
--- End diff --

Since LogicalOperation is generic, do you think we should put a very Hive 
specific string in that enum?


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[denalex]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83099497
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -396,4 +387,12 @@ private Operation reverseOp(Operation operation) {
 operatorTranslationMap.put(8, Operation.HDOP_LIKE);
 return operatorTranslationMap;
 }
+
+static private Map 
initLogicalOperatorTransMap() {
+Map integerLogicalOperationMap = new 
HashMap<>();
--- End diff --

please review 
http://stackoverflow.com/questions/5292790/convert-integer-value-to-matching-java-enum
 for discussion on dangers of relying on ordinal values. I think for us, we 
might want to maintain explicit number as the private member of enum, then 
build and cache the array once for all future access.


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[kavinderd]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83100399
  
--- Diff: 
pxf/pxf-hbase/src/main/java/org/apache/hawq/pxf/plugins/hbase/HBaseFilterBuilder.java
 ---
@@ -52,12 +52,15 @@
  */
 public class HBaseFilterBuilder implements FilterParser.FilterBuilder {
 private Map 
operatorsMap;
+private Map 
logicalOperatorsMap;
 private byte[] startKey;
 private byte[] endKey;
 private HBaseTupleDescription tupleDescription;
+private static final String NOT_OP = "l2";
--- End diff --

Because HBase doesn't support NOT pushdown so we ignore any filters with 
NOT only in HBase


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[kavinderd]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83098043
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -396,4 +387,12 @@ private Operation reverseOp(Operation operation) {
 operatorTranslationMap.put(8, Operation.HDOP_LIKE);
 return operatorTranslationMap;
 }
+
+static private Map 
initLogicalOperatorTransMap() {
+Map integerLogicalOperationMap = new 
HashMap<>();
--- End diff --

@denalex Thanks. I had stumbled on the same thing


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[denalex]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83097860
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -396,4 +387,12 @@ private Operation reverseOp(Operation operation) {
 operatorTranslationMap.put(8, Operation.HDOP_LIKE);
 return operatorTranslationMap;
 }
+
+static private Map 
initLogicalOperatorTransMap() {
+Map integerLogicalOperationMap = new 
HashMap<>();
--- End diff --

check here: 
http://stackoverflow.com/questions/11047756/getting-enum-associated-with-int-value


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[kavinderd]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83097937
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -396,4 +387,12 @@ private Operation reverseOp(Operation operation) {
 operatorTranslationMap.put(8, Operation.HDOP_LIKE);
 return operatorTranslationMap;
 }
+
+static private Map 
initLogicalOperatorTransMap() {
+Map integerLogicalOperationMap = new 
HashMap<>();
--- End diff --

Nvm. Enum.values() returns an array of values


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[kavinderd]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83097391
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -396,4 +387,12 @@ private Operation reverseOp(Operation operation) {
 operatorTranslationMap.put(8, Operation.HDOP_LIKE);
 return operatorTranslationMap;
 }
+
+static private Map 
initLogicalOperatorTransMap() {
+Map integerLogicalOperationMap = new 
HashMap<>();
--- End diff --

I think we'll need to go with an array. I haven't found a way to do a 
reverse lookup of a Enum based on `int` values. Enum.valueOf() takes a String.


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[sansanichfb]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83094033
  
--- Diff: 
pxf/pxf-hive/src/main/java/org/apache/hawq/pxf/plugins/hive/HiveDataFragmenter.java
 ---
@@ -404,24 +411,41 @@ private String buildFilterStringForHive() throws 
Exception {
 HiveFilterBuilder eval = new HiveFilterBuilder(inputData);
 Object filter = eval.getFilterObject(filterInput);
 
-String prefix = "";
-
-if (filter instanceof List) {
-
-for (Object f : (List) filter) {
-if (buildSingleFilter(f, filtersString, prefix)) {
-// Set 'and' operator between each matched partition 
filter.
-prefix = " and ";
-}
-}
-
+if (filter instanceof LogicalFilter) {
+buildCompoundFilter((LogicalFilter) filter, filtersString);
 } else {
-buildSingleFilter(filter, filtersString, prefix);
+buildSingleFilter(filter, filtersString, "");
 }
 
 return filtersString.toString();
 }
 
+private void buildCompoundFilter(LogicalFilter filter, StringBuilder 
filterString) throws Exception{
+String prefix;
+switch(filter.getOperator()) {
--- End diff --

I would add prefix as a member value to LogicalOperation enum and just 
invoke getter here.


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[sansanichfb]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83092740
  
--- Diff: 
pxf/pxf-hive/src/main/java/org/apache/hawq/pxf/plugins/hive/HiveAccessor.java 
---
@@ -205,55 +217,138 @@ private boolean isFiltered(List 
partitionFields,
 return testOneFilter(partitionFields, filter, inputData);
 }
 
-/*
- * We are testing one filter against all the partition fields. The 
filter
- * has the form "fieldA = valueA". The partitions have the form
- * 
partitionOne=valueOne/partitionTwo=ValueTwo/partitionThree=valueThree 1.
- * For a filter to match one of the partitions, lets say partitionA for
- * example, we need: fieldA = partittionOne and valueA = valueOne. If 
this
- * condition occurs, we return true. 2. If fieldA does not match any 
one of
- * the partition fields we also return true, it means we ignore this 
filter
- * because it is not on a partition field. 3. If fieldA = 
partittionOne and
- * valueA != valueOne, then we return false.
- */
-private boolean testOneFilter(List partitionFields,
-  Object filter, InputData input) {
-// Let's look first at the filter
-FilterParser.BasicFilter bFilter = (FilterParser.BasicFilter) 
filter;
+private boolean testForUnsupportedOperators(List filterList) {
+boolean nonAndOp = true;
+for (Object filter : filterList) {
+if (filter instanceof LogicalFilter) {
+if (((LogicalFilter) filter).getOperator() != 
FilterParser.LogicalOperation.HDOP_AND)
+return false;
+if (((LogicalFilter) filter).getFilterList() != null)
+nonAndOp = 
testForUnsupportedOperators(((LogicalFilter) filter).getFilterList());
+}
+}
+return nonAndOp;
+}
 
-boolean isFilterOperationEqual = (bFilter.getOperation() == 
FilterParser.Operation.HDOP_EQ);
-if (!isFilterOperationEqual) /*
+private boolean testForPartitionEquality(List 
partitionFields, List filterList, InputData input) {
+boolean partitionAllowed = true;
+for (Object filter : filterList) {
+if (filter instanceof BasicFilter) {
+BasicFilter bFilter = (BasicFilter) filter;
+boolean isFilterOperationEqual = (bFilter.getOperation() 
== FilterParser.Operation.HDOP_EQ);
+if (!isFilterOperationEqual) /*
   * in case this is not an "equality 
filter"
   * we ignore it here - in partition
   * filtering
   */{
-return true;
-}
+return true;
+}
+
+int filterColumnIndex = bFilter.getColumn().index();
+String filterValue = 
bFilter.getConstant().constant().toString();
+ColumnDescriptor filterColumn = 
input.getColumn(filterColumnIndex);
+String filterColumnName = filterColumn.columnName();
 
-int filterColumnIndex = bFilter.getColumn().index();
-String filterValue = bFilter.getConstant().constant().toString();
-ColumnDescriptor filterColumn = input.getColumn(filterColumnIndex);
-String filterColumnName = filterColumn.columnName();
+for (HivePartition partition : partitionFields) {
+if (filterColumnName.equals(partition.name)) {
 
-for (HivePartition partition : partitionFields) {
-if (filterColumnName.equals(partition.name)) {
 /*
  * the filter field matches a partition field, but the 
values do
  * not match
  */
-return filterValue.equals(partition.val);
-}
-}
+boolean keepPartition = 
filterValue.equals(partition.val);
+
+/*
+ * If the string comparison fails then we should 
check the comparison of
+ * the two operands as typed values
+ */
+if (!keepPartition && 
!partition.val.equals("__HIVE_DEFAULT_PARTITION__")){
--- End diff --

Make "__HIVE_DEFAULT_PARTITION__" a constant?


---
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 

[GitHub] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[sansanichfb]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83090586
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/BasicFilter.java ---
@@ -0,0 +1,37 @@
+package org.apache.hawq.pxf.api;
--- End diff --

Missing Apache header?


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[sansanichfb]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83092607
  
--- Diff: 
pxf/pxf-hive/src/main/java/org/apache/hawq/pxf/plugins/hive/HiveAccessor.java 
---
@@ -205,55 +217,138 @@ private boolean isFiltered(List 
partitionFields,
 return testOneFilter(partitionFields, filter, inputData);
 }
 
-/*
- * We are testing one filter against all the partition fields. The 
filter
- * has the form "fieldA = valueA". The partitions have the form
- * 
partitionOne=valueOne/partitionTwo=ValueTwo/partitionThree=valueThree 1.
- * For a filter to match one of the partitions, lets say partitionA for
- * example, we need: fieldA = partittionOne and valueA = valueOne. If 
this
- * condition occurs, we return true. 2. If fieldA does not match any 
one of
- * the partition fields we also return true, it means we ignore this 
filter
- * because it is not on a partition field. 3. If fieldA = 
partittionOne and
- * valueA != valueOne, then we return false.
- */
-private boolean testOneFilter(List partitionFields,
-  Object filter, InputData input) {
-// Let's look first at the filter
-FilterParser.BasicFilter bFilter = (FilterParser.BasicFilter) 
filter;
+private boolean testForUnsupportedOperators(List filterList) {
--- End diff --

Method name is a bit confusing. Could we rename it to something like 
hasUnsupportedOperator?


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[denalex]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r83044785
  
--- Diff: 
pxf/pxf-hive/src/main/java/org/apache/hawq/pxf/plugins/hive/HiveDataFragmenter.java
 ---
@@ -405,24 +411,41 @@ private String buildFilterStringForHive() throws 
Exception {
 HiveFilterBuilder eval = new HiveFilterBuilder(inputData);
 Object filter = eval.getFilterObject(filterInput);
 
-String prefix = "";
-
-if (filter instanceof List) {
-
-for (Object f : (List) filter) {
-if (buildSingleFilter(f, filtersString, prefix)) {
-// Set 'and' operator between each matched partition 
filter.
-prefix = " and ";
-}
-}
-
+if (filter instanceof LogicalFilter) {
+buildCompoundFilter((LogicalFilter) filter, filtersString);
 } else {
-buildSingleFilter(filter, filtersString, prefix);
+buildSingleFilter(filter, filtersString, "");
 }
 
 return filtersString.toString();
 }
 
+private void buildCompoundFilter(LogicalFilter filter, StringBuilder 
filterString) throws Exception{
+String prefix;
+switch(filter.getOperator()) {
+case HDOP_AND:
+prefix = " and ";
+break;
+case HDOP_OR:
+prefix = " or ";
+break;
+case HDOP_NOT:
+prefix = " not ";
+break;
+default:
+prefix = "";
+}
+
+for (int i = 0; i < filter.getFilterList().size(); i++) {
--- End diff --

for (Object f : filter.getFilterList() )

is more compact notation that collapses 2 lines into one


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[kavinderd]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r82033272
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -396,4 +387,12 @@ private Operation reverseOp(Operation operation) {
 operatorTranslationMap.put(8, Operation.HDOP_LIKE);
 return operatorTranslationMap;
 }
+
+static private Map 
initLogicalOperatorTransMap() {
+Map integerLogicalOperationMap = new 
HashMap<>();
--- End diff --

That's true. I like that idea. @shivzone any objections to @denalex 
suggestion?


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[kavinderd]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r82033359
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -220,6 +193,24 @@ public Object parse(String filter) throws Exception {
 // Store result on stack
 operandsStack.push(result);
 break;
+case 'l':
+LogicalOperation logicalOperation = 
logicalOperationTranslationMap.get(safeToInt(parseNumber()));
+
+if (logicalOperation == null) {
+throw new FilterStringSyntaxException("unknown op 
ending at " + index);
+}
+
+if (logicalOperation == LogicalOperation.HDOP_NOT) {
+Object exp = operandsStack.pop();
+result = filterBuilder.build(logicalOperation, 
exp);
+} else {
--- End diff --

Ok, I'll add the check


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[kavinderd]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r82032792
  
--- Diff: 
pxf/pxf-hbase/src/main/java/org/apache/hawq/pxf/plugins/hbase/HBaseFilterBuilder.java
 ---
@@ -71,14 +74,19 @@ public HBaseFilterBuilder(HBaseTupleDescription 
tupleDescription) {
  * @throws Exception if parsing failed
  */
 public Filter getFilterObject(String filterString) throws Exception {
-FilterParser parser = new FilterParser(this);
-Object result = parser.parse(filterString);
+// First check for NOT, HBase does not support this
+if (filterString.contains(NOT_OP)) {
--- End diff --

Hmm, that's possible. I'll test and fix if 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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[kavinderd]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r82032692
  
--- Diff: 
pxf/pxf-hbase/src/main/java/org/apache/hawq/pxf/plugins/hbase/HBaseFilterBuilder.java
 ---
@@ -71,14 +74,19 @@ public HBaseFilterBuilder(HBaseTupleDescription 
tupleDescription) {
  * @throws Exception if parsing failed
  */
 public Filter getFilterObject(String filterString) throws Exception {
-FilterParser parser = new FilterParser(this);
-Object result = parser.parse(filterString);
+// First check for NOT, HBase does not support this
+if (filterString.contains(NOT_OP)) {
+return null;
+} else {
--- End diff --

Ok, sounds good


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[kavinderd]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r81858427
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -81,6 +88,8 @@
  * @throws Exception if building the filter failed
  */
 public Object build(Operation operation, Object left, Object 
right) throws Exception;
+public Object build(LogicalOperation operation, Object left, 
Object right) throws Exception;
+public Object build(LogicalOperation operation, Object filter) 
throws Exception;
--- End diff --

The reason for having a separate LogicalOperator enum is that fundamentally 
a Logical Operator and a Comparison Operator are different. The grammar for a 
Logical Operators is the following

Logical Operation -> Expr1 (Logical Operator) Expr2
OR
Logical Operation -> (Logical Operator) Expr1  [ This is the NOT case]

Here "Expr" will stand for either a Logical Operation or a Conditional One

A Conditional Operator will have the following form: (Note: they can be in 
reverse form)

Conditional Operation -> Column (Conditional Operator) Constant

So when it comes to parsing a Filter, a Logical Operator will always have 
1/2 children that are themselves an Expression. Whereas a Conditional Operation 
will always have 2 (primitive or constant) children and those children will 
always be leaf nodes.

Thus the reason for having separate op codes is to have a clear way of 
distinguishing Logical Operations from Conditional Ones and internally the 
changes of this Pull Request introduce two Java Objects `Basic Filter` and 
`Logical Filter`. A `BasicFilter` can only have a `column` and `constant` as 
fields but a `LogicalFilter` has a list of subexpressions (`filterList`) as a 
field.

All of the above doesn't explain why we couldn't have a unified enum, but I 
hope it better explains why we felt that the two cases are different enough to 
warrant a separate code. Additionally, when we build the filter string in HAWQ 
itself HAWQ also has two separate Expr types `OpExpr` and `BoolExpr` so our 
approach follows this convention.


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[denalex]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r81829765
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -81,6 +88,8 @@
  * @throws Exception if building the filter failed
  */
 public Object build(Operation operation, Object left, Object 
right) throws Exception;
+public Object build(LogicalOperation operation, Object left, 
Object right) throws Exception;
+public Object build(LogicalOperation operation, Object filter) 
throws Exception;
--- End diff --

This is confusing -- a LogicalOperation can be assumed a type of Operation, 
but it is not -- a different Enum and no inheritance. Why do we need such a 
distinction ? 

If we really need to keep track of whether an operation is logical or not, 
have a boolean flag on the enum value.

If we consolidate, we can have just one Enum, with numbers for mapping and 
a flag for logical (if needed). This will reduce number of functions / enums 
and code overall.


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[denalex]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r81827921
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -396,4 +387,12 @@ private Operation reverseOp(Operation operation) {
 operatorTranslationMap.put(8, Operation.HDOP_LIKE);
 return operatorTranslationMap;
 }
+
+static private Map 
initLogicalOperatorTransMap() {
+Map integerLogicalOperationMap = new 
HashMap<>();
--- End diff --

array will be a better structure for faster access of lookup of integer 
from 0..N to an object.


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[shivzone]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r79487522
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -81,6 +88,8 @@
  * @throws Exception if building the filter failed
  */
 public Object build(Operation operation, Object left, Object 
right) throws Exception;
+public Object build(LogicalOperation operation, Object left, 
Object right) throws Exception;
--- End diff --

Since these are functions in an interface, each function needs to have a 
well defnined javadoc 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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[shivzone]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r79487950
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -220,6 +193,24 @@ public Object parse(String filter) throws Exception {
 // Store result on stack
 operandsStack.push(result);
 break;
+case 'l':
+LogicalOperation logicalOperation = 
logicalOperationTranslationMap.get(safeToInt(parseNumber()));
--- End diff --

Add comment above this mentioning this is to handle logical operators


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[shivzone]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r79487252
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -396,4 +387,12 @@ private Operation reverseOp(Operation operation) {
 operatorTranslationMap.put(8, Operation.HDOP_LIKE);
 return operatorTranslationMap;
 }
+
+static private Map 
initLogicalOperatorTransMap() {
+Map integerLogicalOperationMap = new 
HashMap<>();
--- End diff --

rename this object to  logicalOperatorTransloationMap to stay in sync with 
convnetion used in other function


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[shivzone]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r79488894
  
--- Diff: 
pxf/pxf-hive/src/test/java/org/apache/hawq/pxf/plugins/hive/HiveORCSearchArgumentExample.java
 ---
@@ -0,0 +1,84 @@
+package org.apache.hawq.pxf.plugins.hive;
+
+import org.apache.hadoop.hive.ql.io.sarg.SearchArgument;
+import org.apache.hadoop.hive.ql.io.sarg.SearchArgumentFactory;
+import org.apache.hawq.pxf.api.BasicFilter;
+import org.apache.hawq.pxf.api.LogicalFilter;
+import org.apache.hawq.pxf.api.utilities.ColumnDescriptor;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.List;
+
+public class HiveORCSearchArgumentExample {
--- End diff --

Rename this class as HiveORCSearchArgumentTest. All of the test classes end 
with name ***Test


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[shivzone]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r79486684
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -220,6 +193,24 @@ public Object parse(String filter) throws Exception {
 // Store result on stack
 operandsStack.push(result);
 break;
+case 'l':
+LogicalOperation logicalOperation = 
logicalOperationTranslationMap.get(safeToInt(parseNumber()));
+
+if (logicalOperation == null) {
+throw new FilterStringSyntaxException("unknown op 
ending at " + index);
+}
+
+if (logicalOperation == LogicalOperation.HDOP_NOT) {
+Object exp = operandsStack.pop();
+result = filterBuilder.build(logicalOperation, 
exp);
+} else {
--- End diff --

might be good to explicitly check for enum values here just in case we add 
support for more logical operations. If not atleast add a comment here about 
the logical operations being handled in this case


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[shivzone]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r79488252
  
--- Diff: 
pxf/pxf-hbase/src/main/java/org/apache/hawq/pxf/plugins/hbase/HBaseFilterBuilder.java
 ---
@@ -52,12 +52,15 @@
  */
 public class HBaseFilterBuilder implements FilterParser.FilterBuilder {
 private Map 
operatorsMap;
+private Map 
logicalOperatorsMap;
 private byte[] startKey;
 private byte[] endKey;
 private HBaseTupleDescription tupleDescription;
+private static final String NOT_OP = "l2";
--- End diff --

Why is this explcitly defined here unlike other operators ?


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[shivzone]

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

https://github.com/apache/incubator-hawq/pull/906#discussion_r79445438
  
--- Diff: 
pxf/pxf-api/src/main/java/org/apache/hawq/pxf/api/FilterParser.java ---
@@ -65,6 +66,12 @@
 HDOP_LIKE
 }
 
+public enum LogicalOperation {
--- End diff --

Please add a comment above this enum explaining this enum and maybe update 
the comment above Operation enum to explicitly state that it is for Comparison 
Operators


---
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] incubator-hawq pull request #906: HAWQ-964. Support for OR and NOT Logical O...

[kavinderd]

GitHub user kavinderd opened a pull request:

https://github.com/apache/incubator-hawq/pull/906

HAWQ-964. Support for OR and NOT Logical Operators

Signed-off-by: Leslie Chang 

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

$ git pull https://github.com/kavinderd/incubator-hawq HAWQ-964

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

https://github.com/apache/incubator-hawq/pull/906.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 #906


commit fec6e3055f6b42b5932cbbe34672282d93790e6c
Author: Kavinder Dhaliwal 
Date:   2016-09-15T17:56:20Z

HAWQ-964. Support for OR and NOT Logical Operators

Signed-off-by: Leslie Chang 




---
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.
---