Skip to content

Conversation

zvonand
Copy link
Collaborator

@zvonand zvonand commented Aug 13, 2025

Closes #874.

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Expose IcebergS3 partition_key and sorting_key in system.tables

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All with Aarch64
  • All Regression
  • Disable CI Cache

Copy link

github-actions bot commented Aug 13, 2025

Workflow [PR], commit [c082b3b]

@zvonand zvonand changed the title process partition key and sorting key from IcebergS3 Antalya 25.6.5: Expose IcebergS3 partition_key and sorting_key in system.tables Aug 13, 2025
@zvonand zvonand force-pushed the improvement/antalya/874 branch 2 times, most recently from 099c984 to f55dd06 Compare August 13, 2025 18:45
Copy link

@ianton-ru ianton-ru left a comment

Choose a reason for hiding this comment

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

Is it possible to add some test?

if (!schemas)
return map;

for (size_t i = 0; i < schemas->size(); ++i)

Choose a reason for hiding this comment

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

for (const auto & schema : *schemas)
{
  if (!schema->has("schema-id"))
....

and the same way in other cycles over JSON::Array

Copy link
Collaborator Author

@zvonand zvonand Aug 19, 2025

Choose a reason for hiding this comment

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

Does not look any better: in range-based loop, type will be wrong and will need to be converted explicitly later. This will not be better than it is already:

for (const auto & v : *schemas)
{
    const auto & schema = v.extract<Poco::JSON::Object::Ptr>();

String result;
if (base == "identity")
result = col;
else if (base == "year" || base == "month" || base == "day" || base == "hour")

Choose a reason for hiding this comment

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

Am I right that these types can't have param?
If it true, than next block base != "void" make the same.
If it false, than we lost param, is it correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Am I right that these types can't have param?

Yes, correct

@zvonand zvonand requested a review from Enmk September 9, 2025 11:48
IdToName buildIdToNameMap(const Poco::JSON::Object::Ptr & metadata_obj)
{
IdToName map;
if (!metadata_obj || !metadata_obj->has("current-schema-id") || !metadata_obj->has("schemas"))
Copy link
Member

@Enmk Enmk Sep 9, 2025

Choose a reason for hiding this comment

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

Which docs/sample metadata object this implementation is based on?
Is it just https://iceberg.apache.org/spec/?h=schema+id#table-metadata-fields or is there anything else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, based on docs and test

@Enmk Enmk merged commit 1633469 into antalya-25.6.5 Sep 15, 2025
129 of 136 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants