misc: Prepare for time type extension

[rui-mo]

Preparation for extending the time type to support additional precisions and
timezone behaviors. Add TIME type checks to ensure existing functionality
remains unaffected until it is adapted to support other time types.

TODOs: #16660.

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	6f3c92d
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/69b3dfbd989fb00008197553

[Yuhta]

/claude-review Is the new code functionality equivalent to current code?

[github-actions]

✅ Claude Code Review

Requested by @Yuhta

---

Positive Observations

1. Well-scoped change: The modifications are appropriately limited and focused on TIME type handling
2. Defensive programming: The DCHECK assertions provide good guardrails during development
3. Consistent commenting: The PR description clearly explains this is preparation for future TIME type extensions
4. Type safety: The changes maintain type safety while preparing for future extensions
5. Singleton pattern preserved: The changes maintain the existing singleton pattern for TIME types

Answer to Reviewer Question

"Is the new code functionality equivalent to current code?"

Yes, the new code is functionally equivalent to the current code. Both isTime() and equivalent(*TIME()) perform identical pointer comparisons for the current TimeType singleton implementation:

• isTime(): return (this == TIME().get());
• TimeType::equivalent(other): return this == &other;

The behavioral equivalence is maintained, and the changes serve as preparation for future TIME type extensions that may support multiple precision/timezone variants where the distinction between isTime() and equivalent(*TIME()) will become meaningful.

---

ℹ️ About this response

This review was generated by Claude Code. It analyzed the PR diff and codebase to provide feedback.

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Available commands:

• /claude-review [additional context] - Request a code review. Optionally provide additional instructions (e.g., /claude-review Please focus on memory safety)
• /claude-query <question> - Ask a question about the PR or codebase

[Yuhta]

/claude-query Can you find places in code base where this change might miss the VELOX_DCHECK?

[github-actions]

✅ Claude Response

Requested by @Yuhta

---

Based on my analysis of the codebase, I found the following places where VELOX_DCHECK(type->equivalent(*TIME())); should be added but are missing from the PR:

Files Missing VELOX_DCHECK:

1. /home/runner/work/velox/velox/velox/vector/arrow/Bridge.cpp - Multiple locations:

• Line ~297: if (type->isTime()) in exportArrowFormatStr
• Line ~748: } else if (type->isTime()) in getArrowElementSize
• Line ~783: } else if (type->kind() == TypeKind::BIGINT && type->isTime()) in exportValues
• Line ~2260: } else if (type->isTime()) in importFromArrowImpl
• Lines ~649, ~652: Two locations in isZeroCopyReadSupported function

2. No other Velox Type system files appear to be missing the check.

Note that the files in /home/runner/work/velox/velox/velox/dwio/parquet/writer/arrow/Types.cpp use a different LogicalType class (Parquet's Arrow writer types), not Velox's Type system, so they don't need this check.

The test files in VectorFuzzerTest.cpp were correctly updated in the PR to use equivalent(*TIME()) instead of isTime() for assertions, which is the right pattern for tests.

The most significant missing location is /home/runner/work/velox/velox/velox/vector/arrow/Bridge.cpp which has 6 places where isTime() is called on Velox types but lacks the VELOX_DCHECK.

---

ℹ️ About this response

This response was generated by Claude Code. It analyzed the PR and codebase to answer your question.

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Available commands:

• /claude-review [additional context] - Request a code review. Optionally provide additional instructions (e.g., /claude-review Please focus on memory safety)
• /claude-query <question> - Ask a question about the PR or codebase

[rui-mo]

Responses to Claude review:

I intentionally left the two type->isTime() in isFlatScalarZeroCopy unchanged, because the current logic will disable zero-copy for all time types. Since importing and exporting between int32 and int64 time types of Arrow is not yet fully supported, I assume it is safer to keep zero-copy disabled for all time types.

For the other locations mentioned, the DCHECK is actually present.

[Yuhta]

@rui-mo Maybe just add them there to be safe now even if that code path is eventually going to be deleted?

[rui-mo]

@Yuhta I updated following your suggestion, thanks. The UT failure is irrelevant and is tracked in #16723.

[Yuhta]

/claude-query Can you take another pass any remaining place in code base might miss the VELOX_DCHECK?

[github-actions]

✅ Claude Response

Requested by @Yuhta

---

Based on my analysis, the PR appears to have covered the main places that need VELOX_DCHECK(type->equivalent(*TIME())). The remaining isTime() usages in the codebase are either:

1. Negative checks (like !type->isTime()) - these don't need DCHECK
2. Test assertions - these should likely be updated to use type->equivalent(*TIME()) instead
3. Non-operational code paths - places that don't directly manipulate TIME values

The test files in velox/vector/fuzzer/tests/VectorFuzzerTest.cpp should probably be updated to use equivalent(*TIME()) rather than adding VELOX_DCHECK, since they're testing the type itself.

All the critical operational code paths appear to be covered by the current PR.

---

ℹ️ About this response

This response was generated by Claude Code. It analyzed the PR and codebase to answer your question.

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Available commands:

• /claude-review [additional context] - Request a code review. Optionally provide additional instructions (e.g., /claude-review Please focus on memory safety)
• /claude-query <question> - Ask a question about the PR or codebase