Skip to content
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

Merged
merged 1 commit into from
May 7, 2024

Conversation

ebellm
Copy link
Contributor

@ebellm ebellm commented May 7, 2024

No description provided.

@ebellm ebellm requested a review from kfindeisen May 7, 2024 18:43
Copy link
Member

@kfindeisen kfindeisen left a 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"
Copy link
Member

@kfindeisen kfindeisen May 7, 2024

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Member

@kfindeisen kfindeisen May 7, 2024

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.

Copy link
Contributor

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.

Copy link
Member

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.

@gpdf
Copy link
Collaborator

gpdf commented May 7, 2024

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?

@ebellm
Copy link
Contributor Author

ebellm commented May 7, 2024

@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.

@ebellm ebellm merged commit 0da4264 into main May 7, 2024
4 checks passed
@ebellm ebellm deleted the tickets/DM-44092 branch May 7, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants