Skip to content

Conversation

@sarankk
Copy link
Contributor

@sarankk sarankk commented Nov 13, 2025

No description provided.

Copy link
Contributor

@yifan-c yifan-c left a comment

Choose a reason for hiding this comment

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

I do not see the validation that check the target table is using TWCS when the time range filter is configured. Should you add the check?

Copy link
Contributor

@yifan-c yifan-c left a comment

Choose a reason for hiding this comment

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

One last comment

Comment on lines 35 to 42
public static final SSTableTimeRangeFilter EMPTY = new SSTableTimeRangeFilter(Range.closed(0L, 0L))
{
@Override
public boolean overlaps(long startMicros, long endMicros)
{
return true; // accepts all SSTables
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

When I originally suggested, the empty filter was with range Range.openClosed(0L, 0L). The filter does not overlap with any sstables, hence it is named EMPTY. It is to replace the usage of null.
The code here creates a filter that overlaps with all SSTables. Now I realize that this is probably the behavior you want. Could you rename the filter? It is not really EMPTY, but ALL. The code can be simplified to

Suggested change
public static final SSTableTimeRangeFilter EMPTY = new SSTableTimeRangeFilter(Range.closed(0L, 0L))
{
@Override
public boolean overlaps(long startMicros, long endMicros)
{
return true; // accepts all SSTables
}
};
public static final SSTableTimeRangeFilter ALL = new SSTableTimeRangeFilter(Range.all());

@sarankk sarankk requested a review from yifan-c November 19, 2025 05:01
@sarankk sarankk merged commit 3f36653 into apache:trunk Nov 20, 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.

2 participants