Rest Catalog: Add RESTful AppendFiles data operation #9292
Conversation
geruh
changed the title
| action: | ||
| type: string | ||
| enum: [ "append-files" ] | ||
| appended-manifests: |
There was a problem hiding this comment.
I think we did not really finalize in the discussion threads if this is a good idea. I think it's an easy integration with existing JVM-based engines since they already know how to produce manifests, but if our goal is to make it easy for other non-JVM engines to integrate, seems like this should be a list of DataFiles, or data file locations.
There was a problem hiding this comment.
Hey Jack, you make a good point. The intentions behind the appended-manifest was to showcase the need for compression, and it was a middle ground between a List of DataFiles and a ManifestList. In the case of a large scale Append operation this list can become very large therefore, we can trade some overhead for these operations. However, at the end of the day this is a list of string locations. How the service appends these files is depending on it's ability to read DataFiles into their service from the Request body.
Some other potential approaches we could take are:
- Pagination/batching
- Compression (GZIP, Brotli)
- File Upload of a different type other than avro.
There was a problem hiding this comment.
I added a similar comment about what content to add below, but I think it's best to choose the lowest common denominator (ContentFile/data_file) since that would reduce the need to rewrite manifests to properly define things like the sequence number and alleviates the need for a client to know how to construct a manifest.
github-actions
bot
added
API
spark
parquet
arrow
data
flink
MR
ORC
INFRA
docs
build
hive
AWS
NESSIE
Specification
geruh
force-pushed
the
rest-append-files
branch
from
7f20e5b
to
d914606
Compare
github-actions
bot
removed
API
MR
ORC
INFRA
AWS
NESSIE
DELL
SNOWFLAKE
KAFKACONNECT
labels
| @@ -490,4 +491,21 @@ public void applyTo(ViewMetadata.Builder viewMetadataBuilder) { | |||
| viewMetadataBuilder.setCurrentVersionId(versionId); | |||
| } | |||
| } | |||
|
|
|||
| class AppendFilesUpdate implements MetadataUpdate { | |||
There was a problem hiding this comment.
We might want to rename this to AppendManifestsUpdateAppendContentFileUpdate as appending files would be more of a DataFile type object.
There was a problem hiding this comment.
Actually, I think it makes more sense to pass DataFile entries through this interface since that will actually handle both cases. There are issues with passing manifests because they will need to be rewritten in certain cases. ContentFileParser already supports a json representation, so I think we should pass the data file records through this interface directly (as opposed to just pointing to files).
There was a problem hiding this comment.
I think it makes more sense to pass DataFile entries through this interface since that will actually handle both cases
+1. I think using manifest was more for scalability considerations because there might be many data files. But that might be an over-optimization, and start with passing data file entries is a better starting point.
There was a problem hiding this comment.
For the naming, should this be more like AddContentFileUpdate instead of AppendContentFileUpdate in this case? @danielcweeks given this can be used to add both data and delete files which are both content files, and "append delete files" does not sound like the right terminology.
There was a problem hiding this comment.
I think we need to initially disallow delete files from being added because there are more concerns around achieving correct semantics. We could split this into AppendDataFileUpdate and if we later decide to support deletes, add AppendDeleteFileUpdate.
| @@ -112,6 +111,7 @@ public class RESTSessionCatalog extends BaseViewSessionCatalog | |||
| private static final Logger LOG = LoggerFactory.getLogger(RESTSessionCatalog.class); | |||
| private static final String DEFAULT_FILE_IO_IMPL = "org.apache.iceberg.io.ResolvingFileIO"; | |||
| private static final String REST_METRICS_REPORTING_ENABLED = "rest-metrics-reporting-enabled"; | |||
| private static final String REST_DATA_COMMIT_ENABLED = "rest-data-commit-enabled"; | |||
There was a problem hiding this comment.
A few places we probably need to update the naming.
|
Based on our offline conversations, this is a good starting point, but please add more tests so we can properly merge this PR after reviews. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
Hi All,
I have created this smaller PR focused more on the AppendFiles operations. These changes are stemming from this PR: #9237.
@jackye1995 @amogh-jahagirdar @rdblue