A simple sql to sql rewrite for cardinality()

[nlaptev]

Simplify cardinality on map keys and values functions

A new optimizer rule is added to simplify expressions like
cardinality(map_keys(m)) into cardinality((m)). Same for
map_values function.

[yuanzhanhku]

Might be better to avoid creating a new object when there is no change to the expression.
For example, we can check if the returned pointer of the rewrite is the same as the argument pointer and keep track if any argument is changed.

[nlaptev]

Thanks! We're no longer creating a new object every time

[highker]

For the commit message, we will need to cap each line within 72 characters (inclusive). So something like

Simplify cardinality on map keys and values functions

A new optimizer rule is added to simplify expressions like
`cardinality(map_keys(m))` into `cardinality((m))`. Same for
`map_values` function.

[highker]

Use ImmutableList.Builder<Expression> builder = ImmutableList.builder();

[nlaptev]

done

[highker]

The first condition of if should be outside of the for loop.

[nlaptev]

done -- although it makes the code a little bit longer

[highker]

Assign (FunctionCall) argument) to some variable so it can be reused in the following 3 lines

[nlaptev]

done

[highker]

map_keys and map_values can take and only take 1 argument right?

[highker]

Define a const at the beginning of the class

private static final Set<QualifiedName> MAP_FUNCTIONS = ImmutableSet.of(QualifiedName.of("map_values"), QualifiedName.of("map_keys"));

and use it here

MAP_FUNCTIONS.contains(((FunctionCall) argument).getName())

[nlaptev]

done

[highker]

This will always create new object. We should return new object only when we hit the above written criteria, which I assume most of the queries will not be rewritten with this rule.

[highker]

Can we add map_keys tests?

[nlaptev]

done

[highker]

one argument per line and leave the first line empty

assertRewritten(
    "cardinality(map(ARRAY[cardinality(map_values(m_1)),3], ARRAY[2,cardinality(map_values(m_2))]))",
    "cardinality(map(ARRAY[cardinality(m_1),3], ARRAY[2,cardinality(m_2)]))");

[nlaptev]

done

[highker]

same

[nlaptev]

done

[highker]

Can we add capital cases and mixed cases like CaDInaliTy(....) ?

[nlaptev]

done

[highker]

s/Object/Void

[nlaptev]

done

[highker]

s/tmpFn/functionCall

In presto codebase, we don't usually use abbreviations or acronyms

[nlaptev]

done

[highker]

s/newFnIfRewritten/newFunctionIfRewritten

[nlaptev]

done

[highker]

lol, this part is tricky. It's recursive so that we have to hold on a lot on building new objects before we can tell if we need rewrite or not. cc: @yuanzhanhku

[yuanzhanhku]

Yes, that is a valid concern. The current implementation makes a copy of all function argument pointers recursively even if we don't need to change it. It may cause regressions for queries with lots of expressions. That being said, I don't have an easy way to address this. One idea is to have a tree node level bitmap encode what type of nodes are contained in the tree so that we could have a O(1) function to tell if a tree contains a given node type. But this requires some large refactor.