Impro/type resolver #760

yujiaaa2019 · 2025-09-18T23:34:54Z

No description provided.

Copilot

Pull Request Overview

Adds support for mapping additional Java time/utility types (Instant, UUID, LocalDate) in HollowObjectTypeMapper along with corresponding read/write logic and tests. Introduces new MappedFieldType enum entries and special handling for multi-field reconstruction plus test coverage for serialization round-trips.

Add special wrapper handling for Instant, UUID, LocalDate (schema mapping, copy, parse).
Extend tests to validate new types individually and inside a composite type.
Introduce dedicated test POJO TypeWithSpecialTypes.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
HollowObjectMapperTest.java	Adds tests for Instant, UUID, and composite type containing special types.
HollowObjectMapperHollowRecordParserTest.java	Extends wrapper types test to include Instant.
TypeWithSpecialTypes.java	New test POJO holding Date, Instant, UUID, LocalDate.
HollowObjectTypeMapper.java	Core changes: new special wrapper set, parsing/copy logic, new enum constants for multi-field types.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-18T23:38:05Z

hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java

+                    int nanoPosInPojoSchema = schema.getPosition(MappedFieldType.INSTANT_SECONDS.getSpecialFieldName());
+                    if(nanoPosInPojoSchema != -1) {
+                        int nanoValue = objectNode.getFieldValueInt(MappedFieldType.INSTANT_SECONDS.getSpecialFieldName());
+                        if(nanoValue != Long.MIN_VALUE) {


The nanos branch incorrectly uses INSTANT_SECONDS for both the position lookup and field access, and compares an int to Long.MIN_VALUE instead of Integer.MIN_VALUE. Use INSTANT_NANOS and compare against Integer.MIN_VALUE.

Suggested change

int nanoPosInPojoSchema = schema.getPosition(MappedFieldType.INSTANT_SECONDS.getSpecialFieldName());

if(nanoPosInPojoSchema != -1) {

int nanoValue = objectNode.getFieldValueInt(MappedFieldType.INSTANT_SECONDS.getSpecialFieldName());

if(nanoValue != Long.MIN_VALUE) {

int nanoPosInPojoSchema = schema.getPosition(MappedFieldType.INSTANT_NANOS.getSpecialFieldName());

if(nanoPosInPojoSchema != -1) {

int nanoValue = objectNode.getFieldValueInt(MappedFieldType.INSTANT_NANOS.getSpecialFieldName());

if(nanoValue != Integer.MIN_VALUE) {

Oh yeah this seems like a bug

Copilot · 2025-09-18T23:38:06Z

hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java

                return Long.valueOf(((Date)obj).getTime());
+                //TODO figure out when it's used
+                case UUID_LEAST_SIG_BITS:
+                    return Long.valueOf(((UUID)obj).getLeastSignificantBits());


Only UUID_LEAST_SIG_BITS is handled in retrieveFieldValue; missing cases for UUID_MOST_SIG_BITS, INSTANT_SECONDS, INSTANT_NANOS, LOCAL_DATE_* leading to inconsistent behavior when these fields are requested (they will fall through to default). Add symmetrical handling (or document why not needed) for all newly introduced MappedFieldType entries.

Suggested change

return Long.valueOf(((UUID)obj).getLeastSignificantBits());

return Long.valueOf(((UUID)obj).getLeastSignificantBits());

case UUID_MOST_SIG_BITS:

return Long.valueOf(((UUID)obj).getMostSignificantBits());

case INSTANT_SECONDS:

return Long.valueOf(((Instant)obj).getEpochSecond());

case INSTANT_NANOS:

return Integer.valueOf(((Instant)obj).getNano());

case LOCAL_DATE_YEAR:

return Integer.valueOf(((LocalDate)obj).getYear());

case LOCAL_DATE_MONTH:

return Integer.valueOf(((LocalDate)obj).getMonthValue());

case LOCAL_DATE_DAY:

return Integer.valueOf(((LocalDate)obj).getDayOfMonth());

Copilot · 2025-09-18T23:38:06Z

hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java

+            }
+            else if (SPECIAL_WRAPPERS.contains(clazz)) {


[nitpick] The special wrapper reconstruction logic for UUID, Instant, and LocalDate is duplicated later in parseFlatRecord; consider extracting a shared helper (e.g., rebuildSpecialWrapper(clazz, schema, accessor)) to reduce duplication and risk of divergence (already evident in the Instant nanos bug in the flat record path).

Copilot · 2025-09-18T23:38:07Z

hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java

+                    if (year != null && month != null && day != null) {
+                        obj = LocalDate.of(year, month, day);
+                    }
+                }


[nitpick] The special wrapper reconstruction logic for UUID, Instant, and LocalDate is duplicated later in parseFlatRecord; consider extracting a shared helper (e.g., rebuildSpecialWrapper(clazz, schema, accessor)) to reduce duplication and risk of divergence (already evident in the Instant nanos bug in the flat record path).

Copilot · 2025-09-18T23:38:07Z

hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java

+
+        long time = System.currentTimeMillis();
+        Date date = new Date(time);
+        // what if the DateTimeException is thrown?


[nitpick] The comment is misleading—Instant.now() does not throw DateTimeException under normal circumstances. Remove or clarify to avoid confusion.

Suggested change

// what if the DateTimeException is thrown?

Copilot · 2025-09-18T23:38:07Z

hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java

+        String schemeText = baos.toString();
+        Assert.assertTrue(schemeText.contains("date"));
+        Assert.assertTrue(schemeText.contains("instant"));
+        Assert.assertTrue(schemeText.contains("uuid"));
+        Assert.assertTrue(schemeText.contains("localDate"));


[nitpick] schemeText appears to be a misspelling of schemaText used to hold schema contents; rename to schemaText for accuracy.

Suggested change

String schemeText = baos.toString();

Assert.assertTrue(schemeText.contains("date"));

Assert.assertTrue(schemeText.contains("instant"));

Assert.assertTrue(schemeText.contains("uuid"));

Assert.assertTrue(schemeText.contains("localDate"));

String schemaText = baos.toString();

Assert.assertTrue(schemaText.contains("date"));

Assert.assertTrue(schemaText.contains("instant"));

Assert.assertTrue(schemaText.contains("uuid"));

Assert.assertTrue(schemaText.contains("localDate"));

Sunjeet · 2025-09-23T23:01:13Z

hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java

+                    }
+                    int leastSigBitsPosInPojoSchema = schema.getPosition(MappedFieldType.UUID_LEAST_SIG_BITS.getSpecialFieldName());
+                    if(leastSigBitsPosInPojoSchema != -1) {
+                        long value = hollowObject.getLong(MappedFieldType.UUID_LEAST_SIG_BITS.getSpecialFieldName());


not specifically for this PR, but a note on performance: since we already have a field position here I wonder if we can avoid the string match on field name again cc @eduardoramirez

Sunjeet · 2025-09-23T23:05:02Z

hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java

+                            leastSigBits = value;
+                        }
+                    }
+                    if (mostSigBits != null && leastSigBits != null) {


Since there are 2 long components, having Long.MIN_VALUE as a sentinel effectively means certain UUIDs where one of the long blocks is 2^64 are usable. Effecitvely, 2^64 + 2^64 UUIDs will fall into this edge case, unlikely to ever hit this edge case but wonder if it can be handled better

Sunjeet · 2025-09-23T23:17:15Z

hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java

        ENUM_NAME(FieldType.STRING, "_name"),
-        DATE_TIME(FieldType.LONG, "value");
+        DATE_TIME(FieldType.LONG, "value"),
+        UUID_MOST_SIG_BITS(FieldType.LONG, "mostSigBits"),


we should probably namespace the long components of UUID into their own custom type name, otherwise it will undo the compression benefits for all other long values in the dataset.. since encoding a long normally only #bits required to encode the largest long value, so if UUID is also using the long type then #bits required to encode any long will be 64.

Sunjeet · 2025-09-23T23:18:58Z

hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java

+            catch(Exception e) {
+                throw new RuntimeException(e);
+            }
+        } else if(clazz == UUID.class) {


do we plan to also implement these in the generated HollowAPI? Atm clients would need something like

com.netflix.foo.generated.UUID uuid = someHollowObject.getMyUuidField(); // System.out.println(uuid); // mostSigBits: -8232749396958297039 leastSigBits: -7121165017364359242 java.util.UUID javaUUID = new java.util.UUID(uuid.getMostSigBits(), uuid.getLeastSigBits()); System.out.println(javaUUID); // 8dbf660a-dd7d-4431-9d2c-8a25710f17b6

Sunjeet · 2025-09-23T23:47:19Z

hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java

+                    int nanoPosInPojoSchema = schema.getPosition(MappedFieldType.INSTANT_SECONDS.getSpecialFieldName());
+                    if(nanoPosInPojoSchema != -1) {
+                        int nanoValue = objectNode.getFieldValueInt(MappedFieldType.INSTANT_SECONDS.getSpecialFieldName());
+                        if(nanoValue != Long.MIN_VALUE) {


Oh yeah this seems like a bug

…r all LONGs

yujiaaa2019 · 2025-10-14T19:21:11Z

hollow/src/main/java/com/netflix/hollow/core/read/engine/object/HollowObjectTypeReadState.java

            case FLOAT:
                return (int)fixedLengthValue == HollowObjectWriteRecord.NULL_FLOAT_BITS;
            case DOUBLE:
+            case UUID_LONG:


I added this field for avoiding making all LONGs to take 64 bits. But it will be backward incompatible for namespaces using UUID already.

1. Old producer (LONG) → New consumer (UUID_LONG): The consumer will see a field type mismatch and throw IncompatibleSchemaException 2. New producer (UUID_LONG) → Old consumer (LONG): The old consumer will see a field type mismatch and throw IncompatibleSchemaException

yujiaaa2019 added 2 commits September 18, 2025 15:25

add resolver for Instant, UUID, and LocalDate

d3b9380

remove unused imports

d84940d

yujiaaa2019 requested review from Copilot and eduardoramirez September 18, 2025 23:35

Copilot AI reviewed Sep 18, 2025

View reviewed changes

eduardoramirez requested a review from Sunjeet September 19, 2025 03:47

Sunjeet reviewed Sep 23, 2025

View reviewed changes

yujiaaa2019 added 4 commits October 9, 2025 14:06

Merge branch 'master' into impro/type-resolver

aa31542

add customized field type for UUID LONG to avoid allocating 64bits fo…

632fc9b

…r all LONGs

dont lookup index pos twice for special types

3c0cbd8

add more tests for special types

4ff6c1f

yujiaaa2019 requested a review from Sunjeet October 14, 2025 19:19

yujiaaa2019 commented Oct 14, 2025

View reviewed changes

yujiaaa2019 added 2 commits December 16, 2025 16:29

merge with master

c1c4d34

remove UUID support

c018f5c

-                    return Long.valueOf(((UUID)obj).getLeastSignificantBits());
+                    return Long.valueOf(((UUID)obj).getLeastSignificantBits());
+                case UUID_MOST_SIG_BITS:
+                    return Long.valueOf(((UUID)obj).getMostSignificantBits());
+                case INSTANT_SECONDS:
+                    return Long.valueOf(((Instant)obj).getEpochSecond());
+                case INSTANT_NANOS:
+                    return Integer.valueOf(((Instant)obj).getNano());
+                case LOCAL_DATE_YEAR:
+                    return Integer.valueOf(((LocalDate)obj).getYear());
+                case LOCAL_DATE_MONTH:
+                    return Integer.valueOf(((LocalDate)obj).getMonthValue());
+                case LOCAL_DATE_DAY:
+                    return Integer.valueOf(((LocalDate)obj).getDayOfMonth());

Impro/type resolver #760

Are you sure you want to change the base?

Impro/type resolver #760

Uh oh!

Conversation

yujiaaa2019 commented Sep 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Sunjeet Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Sunjeet Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Sunjeet Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Sunjeet Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Sunjeet Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Sunjeet Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

yujiaaa2019 Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants