Skip to content

fix(c++): make temporal types hashable#3789

Open
BaldDemian wants to merge 1 commit into
apache:mainfrom
BaldDemian:cpp-temporal-keys
Open

fix(c++): make temporal types hashable#3789
BaldDemian wants to merge 1 commit into
apache:mainfrom
BaldDemian:cpp-temporal-keys

Conversation

@BaldDemian

@BaldDemian BaldDemian commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Why?

C++ temporal types (date, duration, timestamp) should be hashable so they can be used as key in unordered_map.

What does this PR do?

  1. Define fory::Timestamp and fory::Duration in cpp/fory/type/temporal.h as wrapper types for std::chrono::time_point and std::chrono::nanoseconds, instead of their aliases. Inside these wrapper types, conversion APIs to and from the chrono representations are provided. I also move fory::Serialization::Date into cpp/fory/type/temporal.h as fory::Date.

  2. Specify std::hash for temporal types (date, duration, timestamp).

  3. Add serializers for raw std::chrono::time_point and std::chrono::nanoseconds.

  4. In type_resolver.h, reject registering chrono temporal types for std::any. chrono nanoseconds and chrono timestamp still work as explicit target types, but dynamic std::any always uses the Fory carriers.

  5. Modify/Add testcases accordingly.

  6. Modify the doc accordingly.

Related issues

Fixes #3787.

AI Contribution Checklist

  • Substantial AI assistance was used in this PR: yes / no
  • If yes, I included a completed AI Contribution Checklist in this PR description and the required AI Usage Disclosure.
  • If yes, my PR description includes the required ai_review summary and screenshot evidence or equivalent persisted links of the final clean AI review results from both fresh reviewers described in AI_POLICY.md, the Fory-guided reviewer and the independent general reviewer, on the current PR diff or current HEAD after the latest code changes.

Does this PR introduce any user-facing change?

Yes. The changes are documented accordingly.

  • Does this PR introduce any public API change?
  • Does this PR introduce any binary protocol compatibility change?

@BaldDemian

BaldDemian commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Hi @chaokunyang, I also noticed that, the doc doesn't specify any message union as map key types:

**Key Type Restrictions:**
- `string` (most common)
- `bool`
- Integer types (`int8`, `int16`, `int32`, `int64`, `uint8`, `uint16`, `uint32`, `uint64`)
- Temporal scalar types (`date`, `timestamp`, `duration`)
- Enums

But the code

INVALID_MAP_KEY_KINDS = {
PrimitiveKind.BYTES,
PrimitiveKind.FLOAT16,
PrimitiveKind.BFLOAT16,
PrimitiveKind.FLOAT32,
PrimitiveKind.FLOAT64,
PrimitiveKind.DECIMAL,

and

def invalid_map_key(field_type: FieldType) -> bool:
if isinstance(field_type, PrimitiveType):
return field_type.kind in INVALID_MAP_KEY_KINDS
return isinstance(field_type, (ListType, ArrayType, MapType))

actually allows any message union as key types.

So which is the intention of Fory's design?

@chaokunyang

Copy link
Copy Markdown
Collaborator

IDL should not allow any/union/message/list/map/float as map keys. Some types are mutable and don't have stable hash. Some types just don't have hash func

/// Timestamp: point in time as nanoseconds since Unix epoch (Jan 1, 1970 UTC)
using Timestamp = std::chrono::time_point<std::chrono::system_clock,
std::chrono::nanoseconds>;
class Timestamp {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

put timestamp and duration into cpp/fory/type/temporal.h and cpp/fory/type/duration.h

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you mean we should put timestamp and duration in a separate header file? Should we include date in that file too?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Create a cpp/fory/type/temporal.h, put duraton/timestamp into it. no cpp/fory/type/duration.h.

I think this is better

/// Serializer for std::chrono::nanoseconds
/// Per xlang spec: serialized as signed varint64 seconds + signed int32
/// nanoseconds
template <> struct Serializer<std::chrono::nanoseconds> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This makes chrono temporal types valid Any registrations for the same internal TypeId as the Fory carriers. Since register_any_type installs any_read_fn on the shared TypeInfo for DURATION/TIMESTAMP, the dynamic deserialization carrier becomes registration-order dependent. We should keep Any/polymorphic reads canonical: DURATION/TIMESTAMP should deserialize to the Fory carrier types, while chrono remains an explicit static target adaptation. Please add coverage for std::any duration/timestamp and prevent chrono registration from overriding the canonical carrier.


/// Duration: absolute length of time as nanoseconds
using Duration = std::chrono::nanoseconds;
class Duration {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Now that Duration/Timestamp are Fory-owned carrier types, they should not be defined in the serializer header/namespace. Please move the value types to cpp/fory/type, expose the canonical API as fory::Duration / fory::Timestamp / fory::Date, and leave aliases in fory::serialization if needed for existing call sites. temporal_serializers.h should only own the Serializer specializations and chrono adapters.

Comment thread docs/guide/cpp/supported-types.md Outdated

## Temporal Types

`Duration`, `Timestamp`, and `Date` are Fory-owned carrier types defined in

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please update the canonical type-mapping docs as part of this public carrier change. The C++ guide now describes Fory-owned carriers, but docs/specification/xlang_type_mapping.md still maps C++ timestamp to std::chrono::nanoseconds. The docs should consistently say that FDL/codegen and dynamic/Any use the Fory carrier by default, while chrono is an explicit C++ adaptation target.

@BaldDemian

Copy link
Copy Markdown
Contributor Author

Hi @chaokunyang, I have addressed all above issues, PTAL. Thanks for your review!

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.

[C++] Generated C++ code fails to compile maps with duration or timestamp keys

2 participants