[GitHub] incubator-hawq pull request #1369: HAWQ-1617. Incorrect processing of boolea...

[benchristel <gi...@git.apache.org>]

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

    https://github.com/apache/incubator-hawq/pull/1369#discussion_r214204676
  
    --- Diff: src/backend/access/external/pxffilters.c ---
    @@ -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))
    --- End diff --
    
    I think the extra parens here can be removed.


---