You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hawq.apache.org by sh...@apache.org on 2018/08/31 14:50:34 UTC
[1/2] incubator-hawq git commit: Fix handling of implicit AND
expressions
Repository: incubator-hawq
Updated Branches:
refs/heads/master ae2af74b2 -> 24e36c935
Fix handling of implicit AND expressions
Co-authored-by: Alex Denissov <ad...@pivotal.io>
Co-authored-by: Shivram Mani <sm...@pivotal.io>
Modify 'pxf_make_expression_items_list()' and 'add_extra_and_expression_items()' so that the latter procedure processes the expression list completely, without any interference with 'pxf_make_expression_items_list()'.
This is identical to https://github.com/greenplum-db/gpdb/commit/3ac93fb4259436c87b6c1705bdb05c003ca93423; see also https://github.com/greenplum-db/gpdb/pull/5470.
Project: http://git-wip-us.apache.org/repos/asf/incubator-hawq/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-hawq/commit/24e36c93
Tree: http://git-wip-us.apache.org/repos/asf/incubator-hawq/tree/24e36c93
Diff: http://git-wip-us.apache.org/repos/asf/incubator-hawq/diff/24e36c93
Branch: refs/heads/master
Commit: 24e36c935c303fd1fdb62cdfa140653e390f3c50
Parents: a12ed20
Author: Ivan Leskin <le...@arenadata.io>
Authored: Fri Aug 31 16:37:13 2018 +0300
Committer: Shivram Mani <sh...@gmail.com>
Committed: Fri Aug 31 07:50:13 2018 -0700
----------------------------------------------------------------------
src/backend/access/external/pxffilters.c | 57 +++++++++++----------------
1 file changed, 24 insertions(+), 33 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/24e36c93/src/backend/access/external/pxffilters.c
----------------------------------------------------------------------
diff --git a/src/backend/access/external/pxffilters.c b/src/backend/access/external/pxffilters.c
index 25d5616..cf76061 100644
--- a/src/backend/access/external/pxffilters.c
+++ b/src/backend/access/external/pxffilters.c
@@ -31,7 +31,7 @@
#include "utils/guc.h"
#include "utils/lsyscache.h"
-static List* pxf_make_expression_items_list(List *quals, Node *parent, int *logicalOpsNum);
+static List* pxf_make_expression_items_list(List *quals, Node *parent);
static void pxf_free_filter(PxfFilterDesc* filter);
static char* pxf_serialize_filter_list(List *filters);
static bool opexpr_to_pxffilter(OpExpr *expr, PxfFilterDesc *filter);
@@ -43,7 +43,7 @@ static bool supported_operator_type_scalar_array_op_expr(Oid type, PxfFilterDesc
static void scalar_const_to_str(Const *constval, StringInfo buf);
static void list_const_to_str(Const *constval, StringInfo buf);
static List* append_attr_from_var(Var* var, List* attrs);
-static void add_extra_and_expression_items(List *expressionItems, int extraAndOperatorsNum, BoolExpr **extraNodePointer);
+static void add_extra_and_expression_items(List *expressionItems, int extraAndOperatorsNum);
static List* get_attrs_from_expr(Expr *expr, bool* expressionIsSupported);
/*
@@ -298,14 +298,15 @@ pxf_free_expression_items_list(List *expressionItems)
*
*/
static List *
-pxf_make_expression_items_list(List *quals, Node *parent, int *logicalOpsNum)
+pxf_make_expression_items_list(List *quals, Node *parent)
{
ExpressionItem *expressionItem = NULL;
List *result = NIL;
ListCell *lc = NULL;
ListCell *ilc = NULL;
+ int quals_size = list_length(quals);
- if (list_length(quals) == 0)
+ if (quals_size == 0)
return NIL;
foreach (lc, quals)
@@ -319,19 +320,22 @@ pxf_make_expression_items_list(List *quals, Node *parent, int *logicalOpsNum)
switch (tag)
{
- case T_Var: // IN(single_value)
+ case T_Var:
+ /* IN(single_value) */
case T_OpExpr:
+ /* Comparison operators >, >=, =, etc */
case T_ScalarArrayOpExpr:
+ /* IN(multiple values) */
case T_NullTest:
{
result = lappend(result, expressionItem);
break;
}
case T_BoolExpr:
+ /* Logical operators AND, OR, NOT */
{
- (*logicalOpsNum)++;
BoolExpr *expr = (BoolExpr *) node;
- List *inner_result = pxf_make_expression_items_list(expr->args, node, logicalOpsNum);
+ List *inner_result = pxf_make_expression_items_list(expr->args, node);
result = list_concat(result, inner_result);
int childNodesNum = 0;
@@ -376,6 +380,14 @@ pxf_make_expression_items_list(List *quals, Node *parent, int *logicalOpsNum)
}
}
+ if ( quals_size > 1 && parent == NULL )
+ {
+ /*
+ If we find more than 1 qualifier, it means we have at least two expressions that are implicitly AND-ed by the query planner. Here, to make it explicit, we will need to add additional AND operators to compensate for the missing ones.
+ */
+ add_extra_and_expression_items(result, quals_size - 1);
+ }
+
return result;
}
@@ -1248,30 +1260,12 @@ serializePxfFilterQuals(List *quals)
if (pxf_enable_filter_pushdown)
{
+ /* expressionItems will contain all the expressions including comparator and logical operators in postfix order */
+ List *expressionItems = pxf_make_expression_items_list(quals, NULL);
- BoolExpr *extraBoolExprNodePointer = NULL;
- int logicalOpsNum = 0;
- List *expressionItems = pxf_make_expression_items_list(quals, NULL, &logicalOpsNum);
-
- /*
- * The 'expressionItems' are always explicitly AND'ed. If there are extra
- * logical operations present, they are the items in the same list. We
- * need to add AND items only for "meaningful" expression items (not
- * including these logical operations)
- */
- if (expressionItems)
- {
- int extraAndOperatorsNum = expressionItems->length - 1 - logicalOpsNum;
-
- add_extra_and_expression_items(expressionItems, extraAndOperatorsNum, &extraBoolExprNodePointer);
- }
-
+ /* result will contain seralized version of the above postfix ordered expressions list */
result = pxf_serialize_filter_list(expressionItems);
- if (extraBoolExprNodePointer)
- {
- pfree(extraBoolExprNodePointer);
- }
pxf_free_expression_items_list(expressionItems);
}
@@ -1282,14 +1276,12 @@ serializePxfFilterQuals(List *quals)
/*
* Adds a given number of AND expression items to an existing list of expression items
- * Note that this will not work if OR operators are introduced
*/
void
-add_extra_and_expression_items(List *expressionItems, int extraAndOperatorsNum, BoolExpr **extraNodePointer)
+add_extra_and_expression_items(List *expressionItems, int extraAndOperatorsNum)
{
- if ((!expressionItems) || (extraAndOperatorsNum < 1))
+ if (!expressionItems || extraAndOperatorsNum < 1)
{
- *extraNodePointer = NULL;
return;
}
@@ -1298,7 +1290,6 @@ add_extra_and_expression_items(List *expressionItems, int extraAndOperatorsNum,
BoolExpr *andExpr = makeNode(BoolExpr);
andExpr->boolop = AND_EXPR;
- *extraNodePointer = andExpr;
andExpressionItem->node = (Node *) andExpr;
andExpressionItem->parent = NULL;
[2/2] incubator-hawq git commit: Fix implicit AND expression addition
Posted by sh...@apache.org.
Fix implicit AND expression addition
Fix implicit addition of extra 'BoolExpr' to a list of expression items.
Before, there was a check that the expression items list did not contain logical operators
(and if it did, no extra implicit AND operators were added).
This behaviour is incorrect. Consider the following query:
SELECT * FROM table_ex WHERE bool1=false AND id1=60003;
Such query will be translated as a list of three items: 'BoolExpr', 'Var' and 'OpExpr'.
Due to the presence of a 'BoolExpr', extra implicit 'BoolExpr' will not be added, and
we get an error "stack is not empty ...".
This commit changes the signatures of some internal functions in pxffilters.c to fix this error.
We pass a number of required extra 'BoolExpr's to 'add_extra_and_expression_items'.
As 'BoolExpr's of different origin may be present in the list of expression items,
the mechanism of freeing the BoolExpr node changes.
The current mechanism of implicit AND expressions addition is suitable only before
OR operators are introduced (in that case, we will have to add ANDs to different parts
of a list, not just the end, as done now).
Project: http://git-wip-us.apache.org/repos/asf/incubator-hawq/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-hawq/commit/a12ed20b
Tree: http://git-wip-us.apache.org/repos/asf/incubator-hawq/tree/a12ed20b
Diff: http://git-wip-us.apache.org/repos/asf/incubator-hawq/diff/a12ed20b
Branch: refs/heads/master
Commit: a12ed20b4e4c366149711dea0aa568f722b2e3e3
Parents: ae2af74
Author: Ivan Leskin <le...@arenadata.io>
Authored: Fri May 25 16:19:36 2018 +0300
Committer: Shivram Mani <sh...@gmail.com>
Committed: Fri Aug 31 07:50:13 2018 -0700
----------------------------------------------------------------------
src/backend/access/external/pxffilters.c | 105 ++++++++++++++------------
1 file changed, 58 insertions(+), 47 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/a12ed20b/src/backend/access/external/pxffilters.c
----------------------------------------------------------------------
diff --git a/src/backend/access/external/pxffilters.c b/src/backend/access/external/pxffilters.c
index 7c1db56..25d5616 100644
--- a/src/backend/access/external/pxffilters.c
+++ b/src/backend/access/external/pxffilters.c
@@ -6,9 +6,9 @@
* 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
@@ -21,7 +21,7 @@
* pxffilters.c
*
* Functions for handling push down of supported scan level filters to PXF.
- *
+ *
*/
#include "access/pxffilters.h"
#include "catalog/pg_operator.h"
@@ -43,7 +43,7 @@ static bool supported_operator_type_scalar_array_op_expr(Oid type, PxfFilterDesc
static void scalar_const_to_str(Const *constval, StringInfo buf);
static void list_const_to_str(Const *constval, StringInfo buf);
static List* append_attr_from_var(Var* var, List* attrs);
-static void enrich_trivial_expression(List *expressionItems);
+static void add_extra_and_expression_items(List *expressionItems, int extraAndOperatorsNum, BoolExpr **extraNodePointer);
static List* get_attrs_from_expr(Expr *expr, bool* expressionIsSupported);
/*
@@ -264,22 +264,21 @@ Oid pxf_supported_types[] =
};
static void
-pxf_free_expression_items_list(List *expressionItems, bool freeBoolExprNodes)
+pxf_free_expression_items_list(List *expressionItems)
{
- ExpressionItem *expressionItem = NULL;
- int previousLength;
+ ExpressionItem *expressionItem = NULL;
while (list_length(expressionItems) > 0)
{
- expressionItem = (ExpressionItem *) lfirst(list_head(expressionItems));
- if (freeBoolExprNodes && nodeTag(expressionItem->node) == T_BoolExpr)
- {
- pfree((BoolExpr *)expressionItem->node);
- }
+ expressionItem = (ExpressionItem *) linitial(expressionItems);
pfree(expressionItem);
- /* to avoid freeing already freed items - delete all occurrences of current expression*/
- previousLength = expressionItems->length + 1;
+ /*
+ * to avoid freeing already freed items - delete all occurrences of
+ * current expression
+ */
+ int previousLength = expressionItems->length + 1;
+
while (expressionItems != NULL && previousLength > expressionItems->length)
{
previousLength = expressionItems->length;
@@ -297,7 +296,6 @@ pxf_free_expression_items_list(List *expressionItems, bool freeBoolExprNodes)
*
* Basically this function just transforms expression tree to Reversed Polish Notation list.
*
- *
*/
static List *
pxf_make_expression_items_list(List *quals, Node *parent, int *logicalOpsNum)
@@ -306,7 +304,7 @@ pxf_make_expression_items_list(List *quals, Node *parent, int *logicalOpsNum)
List *result = NIL;
ListCell *lc = NULL;
ListCell *ilc = NULL;
-
+
if (list_length(quals) == 0)
return NIL;
@@ -377,7 +375,7 @@ pxf_make_expression_items_list(List *quals, Node *parent, int *logicalOpsNum)
break;
}
}
-
+
return result;
}
@@ -872,7 +870,7 @@ append_attr_from_func_args(FuncExpr *expr, List* attrs, bool* expressionIsSuppor
} else if (IsA(node, Var)) {
attrs = append_attr_from_var((Var *) node, attrs);
} else if (IsA(node, OpExpr)) {
- attrs = get_attrs_from_expr((OpExpr *) node, expressionIsSupported);
+ attrs = get_attrs_from_expr((Expr *) node, expressionIsSupported);
} else {
*expressionIsSupported = false;
return NIL;
@@ -1238,64 +1236,77 @@ list_const_to_str(Const *constval, StringInfo buf)
* serializePxfFilterQuals
*
* Wrapper around pxf_make_filter_list -> pxf_serialize_filter_list.
- * Currently the only function exposed to the outside, however
- * we could expose the others in the future if needed.
*
* The function accepts the scan qual list and produces a serialized
* string that represents the push down filters (See called functions
* headers for more information).
*/
-char *serializePxfFilterQuals(List *quals)
+char *
+serializePxfFilterQuals(List *quals)
{
char *result = NULL;
if (pxf_enable_filter_pushdown)
{
- int logicalOpsNum = 0;
- List *expressionItems = pxf_make_expression_items_list(quals, NULL, &logicalOpsNum);
+ BoolExpr *extraBoolExprNodePointer = NULL;
+ int logicalOpsNum = 0;
+ List *expressionItems = pxf_make_expression_items_list(quals, NULL, &logicalOpsNum);
+
+ /*
+ * The 'expressionItems' are always explicitly AND'ed. If there are extra
+ * logical operations present, they are the items in the same list. We
+ * need to add AND items only for "meaningful" expression items (not
+ * including these logical operations)
+ */
+ if (expressionItems)
+ {
+ int extraAndOperatorsNum = expressionItems->length - 1 - logicalOpsNum;
- //Trivial expression means list of OpExpr implicitly ANDed
- bool isTrivialExpression = logicalOpsNum == 0 && expressionItems && expressionItems->length > 1;
+ add_extra_and_expression_items(expressionItems, extraAndOperatorsNum, &extraBoolExprNodePointer);
+ }
- if (isTrivialExpression)
+ result = pxf_serialize_filter_list(expressionItems);
+
+ if (extraBoolExprNodePointer)
{
- enrich_trivial_expression(expressionItems);
+ pfree(extraBoolExprNodePointer);
}
- result = pxf_serialize_filter_list(expressionItems);
- pxf_free_expression_items_list(expressionItems, isTrivialExpression);
+ pxf_free_expression_items_list(expressionItems);
}
-
elog(DEBUG1, "serializePxfFilterQuals: filter result: %s", (result == NULL) ? "null" : result);
return result;
}
/*
- * Takes list of expression items which supposed to be just a list of OpExpr
- * and adds needed number of AND items
- *
+ * Adds a given number of AND expression items to an existing list of expression items
+ * Note that this will not work if OR operators are introduced
*/
-void enrich_trivial_expression(List *expressionItems) {
-
+void
+add_extra_and_expression_items(List *expressionItems, int extraAndOperatorsNum, BoolExpr **extraNodePointer)
+{
+ if ((!expressionItems) || (extraAndOperatorsNum < 1))
+ {
+ *extraNodePointer = NULL;
+ return;
+ }
- int logicalOpsNumNeeded = expressionItems->length - 1;
+ ExpressionItem *andExpressionItem = (ExpressionItem *) palloc0(sizeof(ExpressionItem));
- if (logicalOpsNumNeeded > 0)
- {
- ExpressionItem *andExpressionItem = (ExpressionItem *) palloc0(sizeof(ExpressionItem));
- BoolExpr *andExpr = makeNode(BoolExpr);
+ BoolExpr *andExpr = makeNode(BoolExpr);
- andExpr->boolop = AND_EXPR;
+ andExpr->boolop = AND_EXPR;
+ *extraNodePointer = andExpr;
- andExpressionItem->node = (Node *) andExpr;
- andExpressionItem->parent = NULL;
- andExpressionItem->processed = false;
+ andExpressionItem->node = (Node *) andExpr;
+ andExpressionItem->parent = NULL;
+ andExpressionItem->processed = false;
- for (int i = 0; i < logicalOpsNumNeeded; i++) {
- expressionItems = lappend(expressionItems, andExpressionItem);
- }
+ for (int i = 0; i < extraAndOperatorsNum; i++)
+ {
+ expressionItems = lappend(expressionItems, andExpressionItem);
}
}