Glue: Deduce Iceberg table metadata location if metadata_location not specified

[zvonand]

Also includes #1053 (ClickHouse#87733)

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

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

Deduce Iceberg metadata from Glue's Location

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

[github-actions]

Workflow [PR], commit [0402d8d]

[arthurpassos]

Overall it looks good, just a couple of minor adjustments.

Btw, is it possible to add a test? If it's too hard / we are running against the clock, then it is ok.

[arthurpassos]

This code seems to be duplicated with GlueCatalog::classifyTimestampTZ. Instictively I would ask you to consider creating a function for this, but I am under the impression this PR is not going to upstream, right? Extracting it into a function would make rebasing harder, so it is ok.

[arthurpassos]

Btw, why is it not going into upstream?

[zvonand]

Yes, I'd keep it as is for now. Later I will bring this PR to upstream, and then a small refactor will be done.

[zvonand]

Btw, why is it not going into upstream?

Because we want it in the next 25.8 antalya release ASAP

[arthurpassos]

Consider using S3::URI, I think it offers the functionality you need. All you gotta do is to instantiate it passing the path

[zvonand]

Here again, I am keeping the code that is similar to upstream. This is a good note, I will keep it in mind when making a PR to upstream

[arthurpassos]

Instead of nest try-catch blocks, consider creating two auxiliary functions that return a std::optional

[zvonand]

Here we are trying to read objects that may not exist at all -- for we still need to have try/catch

[arthurpassos]

I know, but instead of having nested try catch blocks, you can have two different functions that return std::optional

Wouldn't something like the below work?

std::optional<std::string> resolveApproach1(...)
{
     try
    {
          // stuff that may throw
          return resolved_path;
    }
    catch(...)
    {
         return std::nullopt;
    }
}

std::optional<std::string> resolveApproach2(...)
{
     try
    {
          // stuff that may throw
          return resolved_path;
    }
    catch(...)
    {
         return std::nullopt;
    }
}

String GlueCatalog::resolveMetadataPathFromTableLocation(const String & table_location, const TableMetadata & table_metadata) const
{
     ....
     if (const auto path = resolveApproach1())
    {
          return path;
     }
     if (const auto path = resolveApproach2())
    {
         return path;
    }

   return std::nulopt;
}

[zvonand]

Yeah, maybe

But I actually do not think it shall be done here (as it is a style and code beauty issue, though I agree that what you suggest looks better). I will probably refactor some of this code when submitting into upstream (e.g. as suggested in your other comments), so I do not see a good reason to spend time on it now.

WDYT?

[arthurpassos]

Sure

[zvonand]

Btw, is it possible to add a test?

Not really. IDK if a good Glue emulator exists. But the thing that existing tests work with != AWS Glue. So, it can only be tested against real AWS.