You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dongjoon-hyun (via GitHub)" <gi...@apache.org> on 2025/08/02 15:23:03 UTC
[PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
dongjoon-hyun opened a new pull request, #51788:
URL: https://github.com/apache/spark/pull/51788
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3146569558
cc @ishnagy , @peter-toth , @LuciferYang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3147420668
Thank you so much, @LuciferYang . Yes, I hope so too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3148496423
For the record, the following is ASF Infra Policy and why this is important.
**ASF INFRA POLICY**
- https://infra.apache.org/github-actions-policy.html
**MONITORING**
- https://infra-reports.apache.org/#ghactions&project=spark&hours=168
**SPARK EFFORT**
- SPARK-48094 Reduce GitHub Action usage according to ASF project allowance
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #51788: [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite`
URL: https://github.com/apache/spark/pull/51788
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3148491165
Thank you all.
I support all approaches to improve this situation including
- the fundamental performance improvements,
- reducing test data size,
- introducing the environment variable because it's better than hard-coded one although we need to rename it to have `SPARK_` prefix like `SPARK_GENERATE_BENCHMARK_FILES`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3148276889
> Could you review this PR if you have some time, @beliefer ?
Thank you for the ping. Disable the test case looks good to me in a short.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3147413166
Merged into master. Thanks @dongjoon-hyun
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3148208712
Thanks @dongjoon-hyun.
@ishnagy do you think we could validate V2 filter implementation with less than 1G items added?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3148337261
> Since the sketch module has virtually no external dependencies, it would be perfectly enough to run any heavy duty bloomfilter tests when the actual BF logic is changed (which is expected to happen quite rarely, I assume).
I've always been curious about this point you mentioned. If we routinely mark it as `@Disabled`, do we have a way to enforce that the submitter or code reviewer manually verifies it when changes occur? If not, when potentially erroneous changes are introduced into a release version, it would result in a poor user experience.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3148348598
@dongjoon-hyun @peter-toth @ishnagy If it indeed requires a large amount of data to reproduce this issue, I have an idea and I'm not sure if everyone agrees:
1. Add an environment variable, such as `VERIFY_SPARK_BF_SUITE`.
2. Replace `Disabled` with `EnabledIfEnvironmentVariable`:
```java
-@Disabled("TODO(SPARK-53077): Re-enable with a resonable test time.")
+@EnabledIfEnvironmentVariable(named = "VERIFY_SPARK_BF_SUITE", matches = "true")
```
This way, we can ensure the test is executed by running `VERIFY_SPARK_BF_SUITE=true build/mvn package --pl common/sketch`, while it will be skipped when running `build/mvn package --pl common/sketch` or `VERIFY_SPARK_BF_SUITE=false build/mvn package --pl common/sketch`.
3. Modify the YAML file for daily tests to ensure that at least one daily test will verify this test.
By doing so, the change pipeline will no longer execute this test, but it will still be verified during the daily test process. This way, even if there are change-related errors, we can detect and correct them within at most one day.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "ishnagy (via GitHub)" <gi...@apache.org>.
ishnagy commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3151275615
I made some measurements with the V1 implementation:
```
100M
testAccuracyRandomDistribution: acceptableFpp(3.000000 %) < actualFpp (3.050257 %) [00m18s] T: ~9.6%
testAccuracyEvenOdd: acceptableFpp(3.000000 %) < actualFpp (3.053887 %) [00m09s] T: ~9.3%
150M
testAccuracyRandomDistribution: acceptableFpp(3.000000 %) < actualFpp (3.080157 %) [00m28s] T: ~15.0%
testAccuracyEvenOdd: acceptableFpp(3.000000 %) < actualFpp (3.079987 %) [00m15s] T: ~15.4%
200M
testAccuracyRandomDistribution: acceptableFpp(3.000000 %) < actualFpp (3.861257 %) [00m37s] T: ~19.8%
testAccuracyEvenOdd: acceptableFpp(3.000000 %) < actualFpp (3.860424 %) [00m20s] T: ~20.6%
250M
testAccuracyRandomDistribution: acceptableFpp(3.000000 %) < actualFpp (3.676172 %) [00m47s] T: ~25.1%
testAccuracyEvenOdd: acceptableFpp(3.000000 %) < actualFpp (3.675387 %) [00m25s] T: ~25.8%
300M
testAccuracyRandomDistribution: acceptableFpp(3.000000 %) < actualFpp (3.210548 %) [00m57s] T: ~30.5%
testAccuracyEvenOdd: acceptableFpp(3.000000 %) < actualFpp (3.209847 %) [00m30s] T: ~30.1%
350M
testAccuracyRandomDistribution: acceptableFpp(3.000000 %) < actualFpp (5.377388 %) [01m07s] T: ~35.8%
testAccuracyEvenOdd: acceptableFpp(3.000000 %) < actualFpp (5.377483 %) [00m36s] T: ~37.1%
400M
testAccuracyRandomDistribution: acceptableFpp(3.000000 %) < actualFpp (8.170380 %) [01m17s] T: ~41.2%
testAccuracyEvenOdd: acceptableFpp(3.000000 %) < actualFpp (8.170716 %) [00m40s] T: ~41.2%
500M
testAccuracyRandomDistribution: acceptableFpp(3.000000 %) < actualFpp (15.392861 %) [01m36s] T: ~51.3%
testAccuracyEvenOdd: acceptableFpp(3.000000 %) < actualFpp (15.391692 %) [00m50s] T: ~51.5%
1G
testAccuracyRandomDistribution: acceptableFpp(3.000000 %) < actualFpp (59.890330 %) [03m07s] T: 100.0%
testAccuracyEvenOdd: acceptableFpp(3.000000 %) < actualFpp (59.888499 %) [01m37s] T: 100.0%
```
I'd say that the truncation bug (with this FPP level) does not cause reliably detectable discrepancies under 350M insertions.
The running time scales approx linearly with the insertion count, so it would mean that with the bare minimum of insertions we could reduce the running time by about a factor of three. It's not nothing (12min -> 4min on github), but 4minutes are still 1-2 orders of magnitudes longer than the old running time.
I saw https://github.com/apache/spark/pull/51806 went in recently, I can create a patch on top of it. Let me know, if you think it still makes sense.
@dongjoon-hyun
@LuciferYang
@peter-toth
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3152400789
Thank you so much, @ishnagy .
It sounds good to me. `4min` will be covered in `non-ansi-build` test pipeline.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "ishnagy (via GitHub)" <gi...@apache.org>.
ishnagy commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3154173925
just for recordkeeping, linking the related PRs together,
I have submitted https://github.com/apache/spark/pull/51845 to reduce the insertion count.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3147012335
Could you review this PR if you have some time, @beliefer ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "ishnagy (via GitHub)" <gi...@apache.org>.
ishnagy commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3148332360
> @ishnagy do you think we could validate V2 filter implementation with less than 1G items added?
the short answer is a (qualified) yes.
the longer answer is a bit more nuanced.
this test was never meant to be run regularly (e.g. as part of a generic pre-merge test suite). I submitted the PR originally having an `@Disabled` annotations on these tests, for the exact purpose of not to hog any general purpose test runs with these longrunning testcases. Since the sketch module has virtually no external dependencies, it would be perfectly enough to run any heavy duty bloomfilter tests when the actual BF logic is changed (which is expected to happen quite rarely, I assume). There was an attempt to start working out some such test classifications in [d9d69801](https://github.com/apache/spark/pull/50933/commits/d9d69801ec7d5fdfd441f12b2f22587a29e3ffd8), but that was quickly cut short.
on the necessity of these tests. I'm a bit worried that forcing this test code into the "generic bucket", getting constrained by the time/resource limitations we have there, significantly cutting down the testcases just to meet those "artificial" constraints, we will introduce some similar blind spots as the original scala suite when it only tested with a 100k insertions at most. This particular int/long truncation problem may be detectable with a 100M elements only, but I think the implementations should not only be proven to be safe in the range we can afford to test regularly (kind of pointlessly), but in 1-2orders of magnitudes higher, which we may not be able run on _every_ change, but we can still afford to run once in a while, when the implementation changes.
- Short term action item: **I'll check if the 100M insertion is enough to demonstrate the problem with the V1 implementation, and prep a PR with a lowered item count (ETA: tomorrow)**
- Mid term: I would be perfectly fine with leaving these tests on disabled (without removing them), even putting back all the testcases we cut out in the chase of decreasing the running time. I'd be happy to work on some tagging facilities, with which we can prevent hogging down the everyday testing without completely disabling the slower tests.
- I'm already working on some performance improvement patches (introducing a non compatible V3 version), which should make the insert/query operations a lot quicker, and thus the high item count tests a lot less painful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53076][CORE][TESTS] Disable `SparkBloomFilterSuite` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #51788:
URL: https://github.com/apache/spark/pull/51788#issuecomment-3147402118
hope this test can be re-enabled within a short period of time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org