HIVE-29312: Concatenate equality conditions in AND nodes #6179
Conversation
|
@zhangbutao @deniskuzZ @dengzhhu653 could you help review this PR? |
|
| } | ||
|
|
||
| @Override | ||
| public void visit(MultiAndLeafNode node) throws MetaException { |
There was a problem hiding this comment.
wonder if Calcite rules could be used for such predicate optimization
There was a problem hiding this comment.
Calcite's seems not have built-in api to combine AND expressions, for example the flattenAnd does not handle OR:
https://github.com/apache/calcite/blob/96b05ee12f936ed057265072ff6a2de8ea0a249e/core/src/main/java/org/apache/calcite/rex/RexUtil.java#L1296-L1325
There was a problem hiding this comment.
why not add it then to Calcite rules? it might optimize Hive queries as well
cc @zabetak, @kasakrisz
There was a problem hiding this comment.
The RexUtil#flattenAnd does exactly what is described in this PR. I don't understand the argument that it doesn't handle OR. How does OR come into the picture? I don't see any mention of OR in the description of this PR/JIRA.
As part of the CBO phase we already use flattenAnd method so in most cases the predicates should be of the form AND(p1,p2,p3,...,pn) and not AND(AND(AND(p1,p2),...,pn). However, these expressions are send as string/bytes from HS2 to HMS and the latter parses them again so not sure how relevant are the optimizations that happen in the HS2 side.
In addition, there may be direct calls to HMS APIs (outside of HS2) and in that case I suppose the expressions can be of any form.
There was a problem hiding this comment.
The RexUtil#flattenAnd does exactly what is described in this PR.
@zabetak you are right, the OR will be add in addAnd function, so it does the same thing like this PR: https://github.com/apache/calcite/blob/96b05ee12f936ed057265072ff6a2de8ea0a249e/core/src/main/java/org/apache/calcite/rex/RexUtil.java#L1322
If we change to use Calcite's rule, we need replace the TreeNode with RexCall.
|
cc @dengzhhu653 |
zabetak
left a comment
zabetak
left a comment
There was a problem hiding this comment.
First of all, I would like to understand a bit better the motivation behind this change to see if the performance improvement justifies the added complexity. I left a question under the JIRA ticket.
| @NotNull List<String> arguments, | ||
| int npartitions) throws TException { | ||
| Table table = client.getTable(dbName, tableName); | ||
| client.addPartitions(createManyPartitions(table, parameters, arguments, npartitions)); |
There was a problem hiding this comment.
Need add partitions in batch because it may hit the max thrift message size for huge partition size.
|
@deniskuzZ @zabetak @dengzhhu653 could you take a look again? The benchmark test |
|
4 participants
What changes were proposed in this pull request?
LeafNodes to aMultiAndLeafNodeMultiAndLeafNodeWhy are the changes needed?
Match longer index prefix in partition filter to improve performance.
For example, a table has partition key:
a, b, c, d, and a filter isa=1 and b=1 and c=1, when visit as a binary treeAND(AND(a=1, b=1), c=1), the filter is:If we combine the adjacent AND conditions, the tree will be
MultiAnd(a=1, b=2, c=3), and the filte can be:PART_NAME like 'a=1/b=2/c=3/%'Does this PR introduce any user-facing change?
No.
How was this patch tested?
Add unit tests and run benchmark test:
java -jar hmsbench.jar -H localhost --savedata /tmp/benchdata --sanitize -N 100 -N 10000 -N 1000000 -o bench_results.csv -C -d testbench_http --params=100 -M 'getPartitionsByFilter.*' --runMode NONACID