feat: Include Parquet Field-IDs on ToSchema

[Fokko]

What changes are proposed in this pull request?

This PR enables setting the Field-ID on a struct. This will be included in the ToSchema schema generation in the form of the parquet.field.id.

How was this change tested?

With a new test

[codecov]

Codecov Report

❌ Patch coverage is 86.25954% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.45%. Comparing base (1ddc026) to head (e270198).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
derive-macros/src/lib.rs	85.47%	12 Missing and 5 partials ⚠️
kernel/src/schema/derive_macro_utils.rs	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1298      +/-   ##
==========================================
+ Coverage   84.42%   84.45%   +0.02%     
==========================================
  Files         112      112              
  Lines       27819    27920     +101     
  Branches    27819    27920     +101     
==========================================
+ Hits        23486    23579      +93     
- Misses       3199     3204       +5     
- Partials     1134     1137       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[scovich]

LGTM, tho it seems like the error checking is a bit baroque?

[scovich]

The leading underscore was... misleading... given that it's actually used.

Also, rule of 30 says this should be a new function that can leverage ?

Also, it's a bit odd that we carefully collect multiple errors (which would imply multiple invalid field_id attributes were specified), but we stop at the first valid one without complaining of duplicates?

Seems like this operation should either stop at the first field id it finds (Result<i64, Error>) or find all field ids (Vec<Result<i64, Error>>) and then blow up unless the list is a single Ok?

[Fokko]

I went with Result<Some(i64), Error> LMKWYT

[zachschuermann]

whoops sorry left an unfinished review, flushing a comment

[OussamaSaoudi]

Hmm I worry that this won't generate the full column mapping data, making this an invalid schema.

Here's an example logical Delta schema:

{
    "name" : "e",
    "type" : {
      "type" : "array",
      "elementType" : {
        "type" : "struct",
        "fields" : [ {
          "name" : "d",
          "type" : "integer",
          "nullable" : false,
          "metadata" : { 
            "delta.columnMapping.id": 5,
            "delta.columnMapping.physicalName": "col-a7f4159c-53be-4cb0-b81a-f7e5240cfc49"
          }
        } ]
      },
      "containsNull" : true
    },
    "nullable" : true,
    "metadata" : { 
      "delta.columnMapping.id": 4,
      "delta.columnMapping.physicalName": "col-5f422f40-de70-45b2-88ab-1d5c90e94db1"
    }
  }

Since name mode is the default, kernel has to decide to convert this schema into one that contains parquet.field.id.

Here's the corresponding physical schema if column mapping mode is ID:

{
    "name" : "col-5f422f40-de70-45b2-88ab-1d5c90e94db1",
    "type" : {
      "type" : "array",
      "elementType" : {
        "type" : "struct",
        "fields" : [ {
          "name" : "col-a7f4159c-53be-4cb0-b81a-f7e5240cfc49",
          "type" : "integer",
          "nullable" : false,
          "metadata" : { 
            "parquet.field.id": 5,
          }
        } ]
      },
      "containsNull" : true
    },
    "nullable" : true,
    "metadata" : { 
      "parquet.field.id": 4,
    }
  }

[OussamaSaoudi]

Setting parquet.field.id directly without checking column mapping mode will likely lead to misuse.

[OussamaSaoudi]

Also, the delta spec requires that both the physical name and column mapping id be present (even if column mapping mode is just one or the other)