-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multicorn agg pushdown v2 #613
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look good apart from minor nitpicks! I'll move on to reviewing the other two PRs now.
query = "SELECT COUNT(*) FROM es.account" | ||
|
||
# Ensure query is going to be aggregated on the foreign server | ||
result = get_engine().run_sql("EXPLAIN " + query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use the local_engine_empty
fixture here (and in other places) instead of get_engine()
. The former rolls back when the test is finished whereas the latter holds open a transaction that might eventually influence other tests. In this case, get_engine()
would return the same global engine object as local_engine_empty
but it's best to be explicit.
assert len(result) == 1 | ||
|
||
# Assert aggregation result | ||
assert result[0] == (Decimal("24.439862542955325"), 49795.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use something like assert math.isclose(result[0][0], 24.4399, rel_tol=1e-05)
here just in case this slightly shifts with different versions of ES to make maintenance easier
assert _extract_queries_from_explain(result)[0] == { | ||
"query": { | ||
"bool": { | ||
"must": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the should
nested inside of must
-- is it so that ES treats the should
clauses as "one of these must match"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct (note that I actually didn't change this logic, it just got exposed in the tests now). The ANY
qual is broken up into logical OR
s (i.e. should
s in ES) and those are AND
ed with the first qual through the must
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I have this weird memory of Multicorn not supporting OR
clauses, but ANY
bypasses that.
@@ -28,6 +29,84 @@ def _extract_queries_from_explain(result): | |||
_bare_sequential_scan = {"query": {"bool": {"must": []}}} | |||
|
|||
|
|||
@pytest.mark.mounting | |||
def test_patern_matching_queries(local_engine_empty): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: pattern
Extend the engine's upperrel pushdown capabilities (#581) with the new features, via splitgraph/Multicorn#2 and splitgraph/postgres-elasticsearch-fdw#2:
WHERE
clauses in combination with aggregationsCOUNT(*)
pushdownCU-1z461e4