CASSANALYTICS-167: Regenerate bloom filters for CQLSSTableWriter by lukasz-antoniak · Pull Request #211 · apache/cassandra-analytics

lukasz-antoniak · 2026-05-27T11:02:03Z

rustyrazorblade · 2026-05-28T20:49:44Z

Solid patch. Just a few things - nothing major.

rustyrazorblade

Just a handful of small questions.

rustyrazorblade · 2026-05-28T20:50:49Z

+            {
+                FileSystemSSTable ssTable = new FileSystemSSTable(dataFile, false, BufferingInputStreamStats::doNothingStats);
+                writerContext.bridge().rebuildBloomFilter(layer.partitioner(), layer.cqlTable(), ssTable, outputDirectory);
+                LOGGER.error("Rebuilt bloom filter for sstable {}", dataFile);


I think this should be INFO level. It'll be confusing to users why every one of their jobs has errors.

rustyrazorblade · 2026-05-28T20:51:47Z

+        String table = cqltable.table();
+
+        Map<MetadataType, MetadataComponent> componentMap = ReaderUtils.deserializeStatsMetadata(keyspace, table, ssTable, EnumSet.of(MetadataType.STATS));
+        StatsMetadata statsMetadata = (StatsMetadata) componentMap.get(MetadataType.STATS);


Is there a possibility this could be NULL?

From what I have reviewed in CQLSSTableWriter, all files (including statistics) have to be present once sstable is considered produced. Did I understand your concern correctly?

Yep, just wanted to be sure.

rustyrazorblade · 2026-05-28T20:57:10Z

+            try (FileOutputStream fos = new FileOutputStream(filterFile, false);
+                 DataOutputStreamPlus stream = new BufferedDataOutputStreamPlus(fos))
+            {
+                BloomFilterSerializer.serialize((BloomFilter) filter, stream);


I'm not sure about this, but if the user has bloom filters disabled, I think this might throw an exception. In 5.0 it uses IFilter.

For 5.0, we are using five-zero bridge and this code.
Adding a condition to skip filter regeneration when bloom_filter_fp_chance == 1.0.

rustyrazorblade · 2026-05-28T20:58:56Z

+                                                                 ImmutableList.of(ImmutableList.of("1", "1"), ImmutableList.of("7", "2")));
+
+        assertThat(bloomFilter.mightContain(searchKeys.get(0))).isTrue();
+        assertThat(bloomFilter.doesNotContain(searchKeys.get(1))).isTrue();


Since we're using a limited set of data in here, it's very unlikely we'll ever hit this... so maybe just leave a comment. If the test changes, we could hit a bloom filter false positive, and it'll cause the test to fail. I know it's really, really unlikely, so please just document it.

rustyrazorblade · 2026-05-28T20:59:40Z

+            }
+            catch (Exception e)
+            {
+                LOGGER.warn("Failed to rebuild bloom filter for sstable {}", dataFile, e);


Is it safe to swallow this exception? How would it fail? Maybe make it an error?

I initially though to ignore failures, but producing unparseable file (and uploading it) may indeed cause an issue. Original 16B file is overwritten, so we do not publish the empty one in case of failure, but potentially broken one.

Ok. Mind just leaving a comment so it's clear that we're deliberately not uploading a bloom filter in case of a failure? I don't expect we'll hit this path ever, I just want it to be clear of the intention.

rustyrazorblade · 2026-05-28T21:05:08Z

 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
+import org.apache.commons.lang.StringUtils;


nit / question: Is this is commons lang3?

yifan-c

looks good overall.

yifan-c · 2026-05-29T22:33:36Z

    }

+    protected void rebuildFilterComponents(@NotNull BulkWriterContext writerContext, @NotNull Path outputDirectory,
+                                         @NotNull DirectoryStream.Filter<Path> filter) throws IOException


nit: indentation is off by 2 spaces.

yifan-c · 2026-05-29T22:56:22Z

+            File filterFile = new File(directory.toFile(), descriptor.relativeFilenameFor(Component.FILTER));
+            try (FileOutputStream fos = new FileOutputStream(filterFile, false);
+                 DataOutputStreamPlus stream = new BufferedDataOutputStreamPlus(fos))
+            {
+                BloomFilterSerializer.serialize((BloomFilter) filter, stream);
+                stream.flush();
+                SyncUtil.sync(fos);
+            }


Should we delete the filterFile if it fails to rebuild, instead of throwing exception?

I think we are fine to upload without the filter component (after deleting the likely corrupted filter file). Server will rebuild the filter. Throwing exception fails the spark task, which causes a more expensive retry, delaying the completion of the spark job.

+1 to throwing it away. Better than having a corrupt one, it'll rebuild on it's own this way.

Done, error handling updated.

lukasz-antoniak · 2026-06-01T07:26:33Z

@rustyrazorblade, @yifan-c, I think I solved all comments. Can you do the final review?

skoppu22 · 2026-06-01T13:39:39Z

        };
        Set<Path> dataFilePaths = new HashSet<>();
        Map<Path, Digest> fileDigests = new HashMap<>();
+        // FIXME: CQLSSTableWriter may produce incomplete Filter.db file, rebuilding it manually (see CASSANALYTICS-167).


If I understand correctly, we are rebuilding complete bloom filter, instead of fixing the actual cause (the cause of empty bloom filter), because the later requires changes in both C* 4.0 and 5.0 and new release/backporting etc in Cassandra, am I right? Can we create a Jira to fix this in upcoming C* versions so we can avoid completely rebuilding for upcoming C* versions, and add corresponding Jira here in comments, so we don't forget by any chance?

skoppu22 · 2026-06-01T13:55:50Z

+    protected void rebuildFilterComponents(@NotNull BulkWriterContext writerContext, @NotNull Path outputDirectory,
+                                           @NotNull DirectoryStream.Filter<Path> filter) throws IOException
+    {
+        LocalDataLayer layer = buildLocalDataLayer(writerContext, outputDirectory, null);


[Minot] LocalDataLayer class's description says 'Mostly used for testing', but we are using for prod. We need to update this comment.

skoppu22 · 2026-06-01T14:04:44Z

+        SchemaBuilder schemaBuilder = new SchemaBuilder(cqltable, partitioner);
+        TableMetadata tableMetadata = schemaBuilder.tableMetaData();
+
+        if (tableMetadata.params.bloomFilterFpChance == 1.0)


I guess here == comparison on doubles is safe as 1.0 doesn't involve rounding

skoppu22 · 2026-06-01T15:03:32Z

+        // rebuild Filter.db files before calculating their digest
+        try (DirectoryStream<Path> stream = getDataFileStream(sstableFilter))
+        {
+            for (Path path : stream)


Can we just pass sstableFilter to rebuildFilterComponents as it is already calling getDataFileStream and looping through it?

lukasz-antoniak force-pushed the CASSANALYTICS-167 branch from 4ad23cb to b8b6c02 Compare May 27, 2026 11:04

lukasz-antoniak marked this pull request as ready for review May 27, 2026 12:50

rustyrazorblade self-requested a review May 27, 2026 14:09

rustyrazorblade reviewed May 28, 2026

View reviewed changes

lukasz-antoniak force-pushed the CASSANALYTICS-167 branch from 0b061e8 to b721c58 Compare May 29, 2026 08:31

lukasz-antoniak added 7 commits May 29, 2026 18:05

CASSANALYTICS-167: Regenerate bloom filters for CQLSSTableWriter

ff347af

Fix digest calculation

bff9ff7

Fix digest calculation

cac61ce

Fix digest calculation

5e3ead3

Fix digest calculation

a44ed86

Apply review comments

5e6d362

Apply review comments

9871ec7

lukasz-antoniak force-pushed the CASSANALYTICS-167 branch from b721c58 to 9871ec7 Compare May 29, 2026 16:06

yifan-c reviewed May 29, 2026

View reviewed changes

lukasz-antoniak added 2 commits June 1, 2026 09:18

Apply review comments

4c3e797

Formatting

34b4156

Fix tests

e58987e

skoppu22 reviewed Jun 1, 2026

View reviewed changes

Conversation

lukasz-antoniak commented May 27, 2026

Uh oh!

rustyrazorblade commented May 28, 2026

Uh oh!

rustyrazorblade left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yifan-c left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yifan-c May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasz-antoniak commented Jun 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skoppu22 Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yifan-c May 29, 2026 •

edited

Loading

skoppu22 Jun 1, 2026 •

edited

Loading