Support COPY command with a TransactionBoundDatabase#3826
Support COPY command with a TransactionBoundDatabase#3826ScottDugas merged 18 commits intoFoundationDB:mainfrom
Conversation
ScottDugas
added
enhancement
ScottDugas
force-pushed
the
copy-with-record-store
branch
from
9521618 to
492ced1
Compare
This requires the database to provide the actual KeySpace under use
This required quite a bit more boiler plate than I would like, but there's not quite enough tests to really warrant generating some sort of pseudo DSL to generate metadata's more efficiently.
The constructor is unused, and anyone should really be interacting with the engine.
ScottDugas
force-pushed
the
copy-with-record-store
branch
from
954fd34 to
068a82e
Compare
This reuses some of the helpers I already had, and adds a couple more for better DRY, and focus of the test code.
| * @throws RelationalException if this catalog does not have a {@link KeySpace} associated | ||
| */ | ||
| @Nonnull | ||
| KeySpace getKeySpace() throws RelationalException; |
There was a problem hiding this comment.
The catalog felt, to me like the most appropriate place to expose this, but I don't have exhaustive knowledge of the state available during query execution, so if there is something more appropriate, let me know.
There was a problem hiding this comment.
The initial API of the catalog considers the KeySpace to be an internal details, that some catalog implementations might rely upon to organize their metadata data, i.e. the RecordLayerStoreCatalog. It feels a bit awkward to me to push the KeySpace to this level.
Could you please consider push this method down to the RecordLayerStoreCatalog level?
There was a problem hiding this comment.
I can't really push it entirely down into RecordLayerStoreCatalog because the point of this change is to allow it to work with a TransactionBoundDatabase and a HollowStoreCatalog. It seems wrong to then have the COPY command be doing multiple different instanceof checks to figure out whether it can support copying.
I did change it to:
public KeySpacePath getKeySpacePath(@Nonnull final URI uri)
which you can see here: ccff9cc
But thinking about it more, I decided that it would be better to have a DataLayout interface, and an implementation based on KeySpace that COPY could use. I'm reticent to put methods on DataLayout because I don't think we have a clear consensus on how we want to handle data layout going forward, and if we diverge from KeySpace, we'll probably want have interfaces in record-layer-core to support it.
That being said, I am working under the assumption that StoreCatalog is intended to be relatively internal, and is only exposed to support TransactionBoundDatabase.
| * {@link com.apple.foundationdb.relational.recordlayer.query.CopyPlan}, and trying to access it will throw a clear | ||
| * error if it is not provided here. | ||
| */ | ||
| public InMemoryCatalog(@Nullable final KeySpace keySpace) { |
There was a problem hiding this comment.
It feels wrong to add the keySpace here since this catalog is a mere in-memory metadata map.
There was a problem hiding this comment.
I think I only added this because 1/3 of the usages had a KeySpace created immediately above it.
I changed it to just not support this, which means it doesn't support COPY, which seems fine.
Outside of testing, I'm not sure why you would use InMemoryCatalog, since the schema is not persisted, you won't be able to read persisted data if the client restarts.
| .add("test_id", UUID.randomUUID()); | ||
|
|
||
| catalog = new InMemoryCatalog(); | ||
| catalog = new InMemoryCatalog(keySpace); |
There was a problem hiding this comment.
Could we create a RecordLayerStoreCatalog with a proper key space instead?
There was a problem hiding this comment.
Since InMemoryCatalog no longer takes a keySpace there are no longer changes here.
- Remove KeySpace parameter to InMemoryCatalog constructor - Cleanup some javadocs - Inline test parameter
ScottDugas
force-pushed
the
copy-with-record-store
branch
from
64d6446 to
535aa14
Compare
2 participants
Breaking Changes
Some TransactionBound classes now require that a keyspace is provided.
TransactionBoundEmbeddedRelationalEngineis still backwards compatible, but some of the more internal classes require a keyspace.Change
This supports using the
COPYcommand with aTransactionBoundDatabase. It requires connecting aKeySpacetoTransactionBoundEmbeddedRelationalEngine.Resolves: #3918