fix: Fix CacheKeyBuilder to correctly handle empty Cacheable keys

[hongkunxu]

Bug
CacheKeyBuilder.appendCacheable had a dead-code bug in its null check. The helper method cacheableToByteArray() returns EMPTY_BYTES (not null) when the input Cacheable is null, so the original guard if (cacheableToByteArray == null) could never match that case. Meanwhile, Cacheable.getCacheKey() is declared @nullable and multiple production classes (e.g., RestrictedDataSource, UnnestDataSource, ExternalDataSource) return null from it to signal "uncacheable." That path happened to work only by coincidence — cacheableToByteArray() returned null implicitly when getCacheKey() returned null, and the == null check caught it.

Fix
Refactored cacheableToByteArray to make null handling explicit:

• cacheable == null (absent component) -> returns EMPTY_BYTES, key stays valid. This preserves backward compatibility with production callers that pass nullable fields like query.getDimensionsFilter() or JoinDataSource.leftFilter.
• cacheable.getCacheKey() == null (uncacheable object) -> returns null, which causes appendCacheable to set invalidKey = true and build() returns null.
• cacheable.getCacheKey() returns an empty array -> throws IllegalArgumentException (unchanged behavior).

Also simplified the empty-key check from Arrays.equals(key, EMPTY_BYTES) to key.length > 0.

Tests
Added two new test cases in CacheKeyBuilderTest:

• testAppendCacheableWithNullInput — verifies that appendCacheable(null) does not invalidate the key.
• testAppendCacheableWithUncacheableObject — verifies that appending a Cacheable whose getCacheKey() returns null causes build() to return null.

Could some committer help to review this PR?
cc @agile @trotter @mcroydon @StFS

[github-actions]

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

[hongkunxu]

Hi @FrankChen021 , Could you please tell me why not merge this PR?

[FrankChen021]

close and repen to trigger the CI @hongkunxu

[github-actions]

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

[github-actions]

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

[hongkunxu]

Hi @FrankChen021 , Could you help move this forward?

[GWphua]

@hongkunxu Thanks for the contribution. Please address the problems that were highlighted in the CI.

Notably,

1. PR title. (prefix title with fix: )
2. Checkstyle (Test with mvn checkstyle:check)

[GWphua]

Hello @hongkunxu

I am rather confused by the dead-code bug you have mentioned. Can you clarify more on how you have met the bug, or is this PR trying to refactor the code to make the null check more explicit?

The new test cases you have introduced are already passing on the old code. I'm not sure if i'm missing something.

[GWphua]

This change is a pure rename right? Can we leave this untouched?

[hongkunxu]

Since this PR has been open for quite some time and I no longer recall the original context clearly, I revisited it and found the changes are unnecessary. I’ve closed the PR. Thanks @GWphua for the review.