SNOW-1654124: Write file name to metadata at the place when we create the file

[sfc-gh-tzhang]

• Write file name to metadata at the place when we create the file (only works for serializeFromJavaObjects)
• Looks like serializeFromParquetWriteBuffers is not maintained to work with primaryFileId as the ParquetWriter is created during setupSchema, we would better remove the code as a whole if we decide to not use it, or a bigger, separated change is required.

[sfc-gh-tzhang]

This is the major change, i move the place where we put the file name

[sfc-gh-hmadan]

wondering if it is possible to defer filename insertion (into metadata) and do it inside InternalStage.put() ?

I'm working on a very related area - uploading to presigned urls for iceberg - and was thinking through cases where the URL has expired and we want to do a retry with a new URL. With the way things are setup right now, if the presigned token has expired I'll end up redoing the buildAndUpload call in the task that FlushService creates with a new URL each time, which will be more wasteful than just reserializing the metadata chunk when a new filename needs to be used.

[sfc-gh-tzhang]

wondering if it is possible to defer filename insertion (into metadata) and do it inside InternalStage.put() ?

I'm working on a very related area - uploading to presigned urls for iceberg - and was thinking through cases where the URL has expired and we want to do a retry with a new URL. With the way things are setup right now, if the presigned token has expired I'll end up redoing the buildAndUpload call in the task that FlushService creates with a new URL each time, which will be more wasteful than just reserializing the metadata chunk when a new filename needs to be used.

It's possible, but let's discuss this outside of this PR? We want to get this in ASAP and create a new release on top.

[sfc-gh-gdoci]

Thanks, fix lgtm. A test to repro the original issue would be good to have.

Re serializeFromParquetWriteBuffers: Good that you bring it up, I'll remove it because we don't plan to enable it.

[sfc-gh-azagrebin]

Maybe I am missing something but I think the test is missing for this change. Otherwise LGTM.

It could be at least write/read test in SDK because there is no way to test it before release with our server side tests.
I could be something like this:
create test buffer -> create flusher from it -> flush to byte[] -> create BdecParquetReader from bytes -> get metadata: fileReader.getFileMetaData().getKeyValueMetaData() (ParquetFileReader.fileReader needs to become a field to get metadata from it)

[sfc-gh-hmadan]

code change looks good, lets add a test before checking it in!

[sfc-gh-tzhang]

Maybe I am missing something but I think the test is missing for this change. Otherwise LGTM.

It could be at least write/read test in SDK because there is no way to test it before release with our server side tests. I could be something like this: create test buffer -> create flusher from it -> flush to byte[] -> create BdecParquetReader from bytes -> get metadata: fileReader.getFileMetaData().getKeyValueMetaData() (ParquetFileReader.fileReader needs to become a field to get metadata from it)

Thanks Andrey! Took me some time but I'm able to add testParquetFileNameMetadata based on your suggestion, PTAL

[sfc-gh-tzhang]

This is needed for ParquetFileReader to work