feat: add find field (by id and name) support to schema #180

nullccxsy · 2025-08-15T10:58:18Z

add insensitive way to find schemafield(list, struct, map)
change the complexity of find name to O(1)
test insensitive way to find schemafield(list, struct, map)

wgtmac

This is a duplicate of #145, perhaps @mapleFU can take a look as well.

src/iceberg/type.h

test/type_test.cc

src/iceberg/type.cc

src/iceberg/type.h

src/iceberg/type.cc

src/iceberg/type.h

src/iceberg/schema.h

src/iceberg/schema.cc

src/iceberg/schema.h

src/iceberg/type.h

src/iceberg/type.cc

src/iceberg/schema.h

src/iceberg/schema.cc

src/iceberg/schema.h

src/iceberg/schema.cc

src/iceberg/schema.h

wgtmac · 2025-08-22T07:49:59Z

src/iceberg/schema.cc

+        NametoIdVisitor::GetPath(path, std::string(field.name()), case_sensitive_);
+    short_full_path = GetPath(short_path, std::string(field.name()), case_sensitive_);
+    name_to_id_[full_path] = field.field_id();
+    name_to_id_.emplace(short_full_path, field.field_id());


Check the return value and error out if failed.

If it fails, it means that the short name conflicts with the canonical name. In this case, the short name is discarded, so don't need to check the return value.

src/iceberg/schema.cc

src/iceberg/schema.h

test/schema_test.cc

wgtmac

Thanks for the update! I think we are close to the finish line.

src/iceberg/schema.cc

test/schema_test.cc

wgtmac · 2025-08-23T08:39:08Z

Please rename to title to feat: add find field (by id and name) support to schema

src/iceberg/schema.cc

wgtmac · 2025-08-23T14:16:08Z

Please note that the java impl has Function<String, String> quotingFunc. This is not required in this PR and we need to make sure the current implementation is not too hard to add the quoting function.

mapleFU · 2025-08-23T14:32:51Z

src/iceberg/schema.h

+  Result<Status> InitIdToIndexMap() const;
+  Result<Status> InitNameToIndexMap() const;
+  Result<Status> InitLowerCaseNameToIndexMap() const;


Are they "Ensure" rather than Init?

May name it LazyIdToIndexMap ?

The purpose here is not "Init" ( Maybe reinitialize ) Mapping, it do Ensure the mapping to be "initialized", I guess EnusreIdToIndexMap() is ok to me.

Generally, LazyIdToIndexMap seems to be another api like Result<const map<...>&> LazyIdToIndexMap

mapleFU

Personally I feel a bit afraid about lazy initialize mapping, since it would be thread-unsafetly. But maybe it's not a severe problem here.

wgtmac · 2025-08-23T14:57:36Z

Personally I feel a bit afraid about lazy initialize mapping, since it would be thread-unsafetly. But maybe it's not a severe problem here.

For lazy initialization, we may leverage std::once_flag for thread-safety.

mapleFU · 2025-08-23T14:59:25Z

For lazy initialization, we may leverage std::once_flag for thread-safety.

So this requires a mutex and three once_flag? It might be huge but it's ok to me...

nullccxsy · 2025-08-24T04:28:05Z

For lazy initialization, we may leverage std::once_flag for thread-safety.

So this requires a mutex and three once_flag? It might be huge but it's ok to me...

ok, this will be fixed

mapleFU · 2025-08-24T05:59:12Z

ok, this will be fixed

We can support this is another patch, currently we can add a todo here.

src/iceberg/schema.h

test/schema_test.cc

src/iceberg/schema.cc

test/schema_test.cc

src/iceberg/schema.cc

src/iceberg/schema.h

test/schema_test.cc

wgtmac

LGTM, thanks @nullccxsy!

src/iceberg/schema.cc

wgtmac · 2025-08-27T08:41:44Z

src/iceberg/schema.h

+///
+/// Enables std::unordered_map to directly accept std::string_view lookup keys
+/// without creating temporary std::string objects, using C++20's transparent lookup.
+struct string_hash {


Do we want to move it to string_utils.h so type.h can reuse it?

Perhaps we can do this altogether in #194

- Add GetFieldByName method to support nested field queries with dot notation - Implement InitNameToIndexMap and InitIdToIndexMap for field mapping - Introduce SchemaFieldVisitor for schema traversal - Implement name pruning logic for element and value in nested structure

- Introduce IdVisitor for schema traversal to init id_to_index_(Map) - Introduce NameVisitor for schema traversal to init name_to_index_(Map), lowercase_name_to_index_(map)

…ava impl 1. Refactor IdVisitor: Add VisitNested() to handle NestedType, make Visit() generic for all Type hierarchy (avoid anti-pattern) 2. Merge id_to_index_ + full_schemafield_ into id_to_field_ (std::unordered_map<int32_t, const ref<SchemaField>>) to reduce memory footprint 3. Align with Java impl's idToField design (support future alias/identifier fields without refactor)

- change the type of id from size_t to int_32 - rename GetPath to BuildPath - fix bug new_short_path should build from short_path, not path

…sult call in macros - Added logic to detect and handle duplicate names in NameToIdVisitor to prevent invalid schema errors. - Fixed duplicate result call issue in src/iceberg/util/macros.h by optimizing ICEBERG_RETURN_UNEXPECTED macro.

… conversion function - Implemented detection for duplicate field IDs, Names in visitors, returning kInvalidSchema error with message. - Updated lowercase conversion in NameToIdVisitor

…reduce copies and unify initialization - Switched field_ and schema_ members in test classes to std::unique_ptr for unique ownership and to avoid duplicate and avoid creating duplicate objects.

… std::string creation

src/iceberg/util/string_util.h

nullccxsy force-pushed the feature/case-insensitive-field-find branch from adc4817 to ec73749 Compare August 15, 2025 11:16

wgtmac reviewed Aug 17, 2025

View reviewed changes

nullccxsy force-pushed the feature/case-insensitive-field-find branch 4 times, most recently from 3b0653a to ae4515e Compare August 20, 2025 14:34

wgtmac reviewed Aug 21, 2025

View reviewed changes

nullccxsy changed the title ~~feat(type): add insensitive way to find schemafield & test~~ feat(type): add insensitive way to find schemafield Aug 21, 2025

nullccxsy changed the title ~~feat(type): add insensitive way to find schemafield~~ feat(schema): add insensitive way to find schemafield Aug 21, 2025

wgtmac suggested changes Aug 21, 2025

View reviewed changes

nullccxsy force-pushed the feature/case-insensitive-field-find branch 2 times, most recently from 0394c01 to ef4b2de Compare August 22, 2025 06:33

nullccxsy requested a review from wgtmac August 22, 2025 07:03

wgtmac suggested changes Aug 22, 2025

View reviewed changes

nullccxsy force-pushed the feature/case-insensitive-field-find branch from 8319453 to 8ba65bf Compare August 23, 2025 05:57

wgtmac mentioned this pull request Aug 23, 2025

feat: basic implement GetFieldByNameCaseInsensitive #145

Closed

wgtmac reviewed Aug 23, 2025

View reviewed changes

nullccxsy commented Aug 23, 2025

View reviewed changes

src/iceberg/schema.cc Outdated Show resolved Hide resolved

nullccxsy changed the title ~~feat(schema): add insensitive way to find schemafield~~ feat: add find field (by id and name) support to schema Aug 23, 2025

mapleFU reviewed Aug 23, 2025

View reviewed changes

nullccxsy force-pushed the feature/case-insensitive-field-find branch 3 times, most recently from 0f85748 to f0dbff4 Compare August 25, 2025 09:28

wgtmac reviewed Aug 25, 2025

View reviewed changes

nullccxsy force-pushed the feature/case-insensitive-field-find branch from 9de800c to c3b4bd3 Compare August 25, 2025 12:28

wgtmac reviewed Aug 25, 2025

View reviewed changes

wgtmac approved these changes Aug 26, 2025

View reviewed changes

dongxiao1198 approved these changes Aug 27, 2025

View reviewed changes

mapleFU reviewed Aug 27, 2025

View reviewed changes

src/iceberg/schema.cc Outdated Show resolved Hide resolved

src/iceberg/schema.cc Show resolved Hide resolved

mapleFU approved these changes Aug 27, 2025

View reviewed changes

nullccxsy force-pushed the feature/case-insensitive-field-find branch 2 times, most recently from d5f8d92 to 4df8eb7 Compare August 27, 2025 08:29

wgtmac reviewed Aug 27, 2025

View reviewed changes

nullccxsy added 10 commits August 27, 2025 18:52

feat(schema): implement nested field lookup with name pruning

f379edc

- Introduce IdVisitor for schema traversal to init id_to_index_(Map) - Introduce NameVisitor for schema traversal to init name_to_index_(Map), lowercase_name_to_index_(map)

style(schema): fix code style issues in schema_test

36f9a96

- change the type of id from size_t to int_32 - rename GetPath to BuildPath - fix bug new_short_path should build from short_path, not path

fix: fix comments, add duplicate field ID detection, update lowercase…

10bb5d4

… conversion function - Implemented detection for duplicate field IDs, Names in visitors, returning kInvalidSchema error with message. - Updated lowercase conversion in NameToIdVisitor

fix: fix comments, optimize smart pointer usage in schema_test.cc to …

92b6db4

…reduce copies and unify initialization - Switched field_ and schema_ members in test classes to std::unique_ptr for unique ownership and to avoid duplicate and avoid creating duplicate objects.

fix: fix comments, add transparent string hash to eliminate temporary…

ef5c1cf

… std::string creation

fix comments

52b53ca

fix comments

9aa221d

nullccxsy force-pushed the feature/case-insensitive-field-find branch from 17bcac2 to 9aa221d Compare August 27, 2025 10:56

wgtmac reviewed Aug 28, 2025

View reviewed changes

src/iceberg/util/string_util.h Outdated Show resolved Hide resolved

fix comments

0e0d42c

feat: add find field (by id and name) support to schema #180

Are you sure you want to change the base?

feat: add find field (by id and name) support to schema #180

Conversation

nullccxsy commented Aug 15, 2025

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

nullccxsy Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac commented Aug 23, 2025

Uh oh!

Uh oh!

wgtmac commented Aug 23, 2025

Uh oh!

mapleFU Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

nullccxsy Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

mapleFU Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Aug 23, 2025