-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-44092: Remove placeholder timeseries feature columns from DIAObject schemas #218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one (predictable) quibble about the version number.
@@ -1,7 +1,7 @@ | |||
--- | |||
name: "ApdbSchema" | |||
"@id": "#apdbSchema" | |||
version: "1.0.0" | |||
version: "1.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it seems like ApdbSchema is using a "fully/partially/non-compatible" approach instead of semantic versioning, I don't think this number is correct -- if you want to argue that it's compatible because the fields were unused, then it should be 1.0.1; if you want to consider the schema on its own merits, it's 2.0.0. I assume the former is closer to your original intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see arguments for either, but @andy-slac had suggested 1.1.0 in a DM so I went with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a complicated issue because it also involves something on client side. Semantic versioning is also hard to apply to database schema (semantic versioning as mostly about API changes). In this case my thinking was that we cannot claim full compatibility because there may be a potential client that will read 1.0.0 schema and try to do something with those columns. Backward compatibility seems safer - client that is configured with 1.1.0 schema can still read and write 1.0.0 database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit worried by the definition of "backward compatibility" implied by that statement.
Normally when people talk about a library (or its API changes) being backward compatible, they mean that old clients can use new versions of the library. That's also what Prompt Processing assumed it meant.
However, you seem to be saying that an APDB change is compatible if new clients can interact with old databases, i.e. forward compatibility in the conventional sense. That's surprising because such compatibility is normally the client's responsibility. It would also mean that in PP we need to be stricter about saying that e.g. a given release can work with schemas 1.0-1.1, but can't guarantee 1.2.
If that's the direction you want compatibility to be defined in, please document that very explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, maybe term "backward" is not quite right here, but it more or less reflects the same direction of upgrades. With API example you upgrade library and keep old client. With schema version - you upgrade yaml schema file but can keep old database schema. I'm not sure one can say who is the client of what here, but upgrades usually happen for yaml schema first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, please make that unambiguous in the docs (in practice, DMTN-269?) so that everybody has the same expectations.
Are these being removed as a first step toward adding back a more concrete realization of these placeholders? Or is the idea itself being retired? |
@gpdf yes, this is a first step toward adding back a more concrete realization of the placeholders. We already have some named timeseries features in DIAObject and expect to add more, but we will store them as named columns rather than a giant BLOB. |
No description provided.