yupeng9 commented on pull request #6096: URL: https://github.com/apache/incubator-pinot/pull/6096#issuecomment-703188624
I don't think so. Strictly speaking, primary key is a constraint defined on the schema, just like other constraints over the schema such as a column is `NOT NULL` or a column is `UNIQUE`. This is well defined in the database literature. If the consideration is to limit its usage, then we can add a checker to ensure it's defined only when Upsert mode is not `NONE`. However, for cleaniness (and also the ease of future development and structuring), the constraints shall be defined as part of the schema. > After some consideration, I still think it is better to wrap the primary key columns inside the `UpsertConfig` for the following reasons: > > * The schema for the offline segment is re-constructed from the segment metadata, where primary key columns might not be maintained > * For regular use cases, there is no primary key concept in Pinot. Putting it in `UpsertConfig` can keep all related configs at the same place > * For the upcoming JOIN feature, we should have another config for the in-memory lookup table, where we can put the primary key there. This 2 features cannot be applied to the same table, and I don't see the benefit of keeping this info inside the schema > > Since this is the first feature using the primary key concept, I suggest putting all the related configs at the same place for easier management. If we find this as a common concept in the future, we can move it then. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
