fix(c++): make temporal types hashable#3789
Conversation
|
Hi @chaokunyang, I also noticed that, the doc doesn't specify fory/docs/compiler/schema-idl.md Lines 1492 to 1498 in 6c6ba3f But the code fory/compiler/fory_compiler/ir/validator.py Lines 40 to 46 in 6c6ba3f and fory/compiler/fory_compiler/ir/validator.py Lines 550 to 553 in 6c6ba3f actually allows So which is the intention of Fory's design? |
|
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 { |
There was a problem hiding this comment.
put timestamp and duration into cpp/fory/type/temporal.h and cpp/fory/type/duration.h
There was a problem hiding this comment.
Do you mean we should put timestamp and duration in a separate header file? Should we include date in that file too?
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
|
|
||
| ## Temporal Types | ||
|
|
||
| `Duration`, `Timestamp`, and `Date` are Fory-owned carrier types defined in |
There was a problem hiding this comment.
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.
4c7ce2c to
7857f92
Compare
|
Hi @chaokunyang, I have addressed all above issues, PTAL. Thanks for your review! |
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?
Define
fory::Timestampandfory::Durationincpp/fory/type/temporal.has wrapper types forstd::chrono::time_pointandstd::chrono::nanoseconds, instead of their aliases. Inside these wrapper types, conversion APIs to and from the chrono representations are provided. I also movefory::Serialization::Dateintocpp/fory/type/temporal.hasfory::Date.Specify
std::hashfor temporal types (date, duration, timestamp).Add serializers for raw
std::chrono::time_pointandstd::chrono::nanoseconds.In
type_resolver.h, reject registeringchronotemporal types forstd::any. chrono nanoseconds and chrono timestamp still work as explicit target types, but dynamicstd::anyalways uses the Fory carriers.Modify/Add testcases accordingly.
Modify the doc accordingly.
Related issues
Fixes #3787.
AI Contribution Checklist
yes/noyes, I included a completed AI Contribution Checklist in this PR description and the requiredAI Usage Disclosure.yes, my PR description includes the requiredai_reviewsummary and screenshot evidence or equivalent persisted links of the final clean AI review results from both fresh reviewers described inAI_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.