Skip to content
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

RONDB-822 pushdown-aggregation bugfix #613

Conversation

KernelMaker
Copy link
Collaborator

Description:
it's not a bug and works fine in the release binary.
The assertion was added to ensure everything behaves as I expected.
Specifically, I wanted to verify that when LQH sends scanfragconf to TC,
no aggregation results remain cached unless:

We are scanning and aggregating an empty table.

No rows meet the filter condition.

It's the end of the scan.

Under these conditions, the scanPtr->scanState should change to
ScanRecord::WAIT_CLOSE_SCAN.

However, this case help us catch an new exception:
When performing an index scan on an empty table, dbtux directly
returns ZEMPTY_FRAGMENT, skipping the state transition from
ScanRecord::WAIT_ACC_SCAN to ScanRecord::WAIT_CLOSE_SCAN. This
behavior causes the assertion to trigger. The root cause lies in the
different ways dbtux and dbtup handle empty tables during
execACC_SCANREQ.

Solution:
Add ScanRecord::WAIT_ACC_SCAN to the assertion to account for this
scenario.

Description:
  it's not a bug and works fine in the release binary.
  The assertion was added to ensure everything behaves as I expected.
  Specifically, I wanted to verify that when LQH sends scanfragconf to TC,
  no aggregation results remain cached unless:

    We are scanning and aggregating an empty table.

    No rows meet the filter condition.

    It's the end of the scan.

  Under these conditions, the scanPtr->scanState should change to
  ScanRecord::WAIT_CLOSE_SCAN.

  However, this case help us catch an new exception:
  When performing an index scan on an empty table, dbtux directly
  returns ZEMPTY_FRAGMENT, skipping the state transition from
  ScanRecord::WAIT_ACC_SCAN to ScanRecord::WAIT_CLOSE_SCAN. This
  behavior causes the assertion to trigger. The root cause lies in the
  different ways dbtux and dbtup handle empty tables during
  execACC_SCANREQ.

Solution:
  Add ScanRecord::WAIT_ACC_SCAN to the assertion to account for this
  scenario.
mronstro
mronstro previously approved these changes Jan 3, 2025
Copy link
Collaborator

@svenssonaxel svenssonaxel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result file has diffs indicating a bug in null handling. I'm not sure whether this bug is in RonSQL or PDAgg. If you don't want to fix the bug, at least remove the diff lines, causing the test case to fail, open a ticket and edit mysql-test/suite/ronsql/disabled.def by updating the ticket number. If you do fix it, the diff lines should be removed when running the case with --record, and you can remove the case from disabled.def.

+++ ronsql.out
@@ -1,2 +1,2 @@
count(*)
-0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff indicates another bug in how null values are handled.

+++ ronsql.out
@@ -1,2 +1,2 @@
count(*)
-0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

+++ ronsql.out
@@ -1,2 +1,2 @@
count(*)
-0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

+++ ronsql.out
@@ -1,2 +1,2 @@
count(*)
-0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

@KernelMaker
Copy link
Collaborator Author

Thank you for the review! @svenssonaxel
Regarding the diff-issue, I think it requires RonSQL to handle the default NULL result based on different aggregation operations. As shown in this example, when performing aggregation on an empty table, COUNT() behaves differently on MySQL compared to other operations in handling NULL.
Since it’s an empty table, the aggregator on the data node doesn’t get a chance to perform any actual calculations. It simply returns the same default aggregation result (NULL) for all operations. This necessitates handling the result on the RonSQL side. Specifically, COUNT(), converts its result to zero instead of leaving it as NULL.
If you agree with it, we can proceed with merging this patch first, and then you can take the next steps based on it.
image

Modify the ronsql_emptytable.result file to match the expected output.
@KernelMaker
Copy link
Collaborator Author

.result updated

@svenssonaxel
Copy link
Collaborator

@KernelMaker Sounds great! Kindly create a ticket and update disabled.def, then we can merge.

@KernelMaker
Copy link
Collaborator Author

@svenssonaxel Done.

@svenssonaxel
Copy link
Collaborator

I believe you meant RONDB-831.

@KernelMaker KernelMaker force-pushed the bugfix_pushdown_aggregation_RONDB-822 branch from e057933 to f1b99c9 Compare January 7, 2025 09:17
@KernelMaker KernelMaker merged commit 0301e6a into logicalclocks:24.10-main Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants