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

Handle TIME data type in the backend #426

Closed
Tracked by #250
kgodey opened this issue Jul 18, 2021 · 7 comments · Fixed by #628
Closed
Tracked by #250

Handle TIME data type in the backend #426

kgodey opened this issue Jul 18, 2021 · 7 comments · Fixed by #628
Assignees
Labels
type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@kgodey
Copy link
Contributor

kgodey commented Jul 18, 2021

This issue is to ensure that Mathesar can handle the Postgres TIME data type.

As part of this issue, we need to ensure that:

  • Users can use the API to change a column to TIME(with time zone) if it's possible to do so.
  • Users can use the API to change a column to TIME(without time zone) if it's possible to do so.
  • Users can set and change the precision of a given TIME column via a type_options field in the API.
  • Automatic type inference during file import suggests TIME when it makes sense to do so.

Additional Context

@kgodey kgodey added ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL work: database labels Jul 18, 2021
@kgodey kgodey added this to the 07. Initial Data Types milestone Jul 18, 2021
@mathemancer
Copy link
Contributor

Automatic type inference during file import suggests TIME when it makes sense to do so.

We should also consider whether the native PostgreSQL parsing suffices: https://www.postgresql.org/docs/13/datatype-datetime.html . My thinking is that:

  • We only want to do that parsing in one place if possible
  • If a user wants to input into the DB itself, it would be nice if they could input with the same strings there as through the UI
  • The PostgreSQL date / time parsing has most (maybe even all; I'd need to compare the docs more closely) of the functionality of that library.

The downside would be that we'd have to do any customization or guardrails needed in SQL, rather than python, which is less convenient to implement and much less convenient to test.

@dmos62
Copy link
Contributor

dmos62 commented Nov 24, 2021

@mathemancer I think that having the backend do parsing that is likely to fail, like parsing human-readable time-related expressions, is problematic because of the roundtrip time: when inputing a time-expression you want the parsing to happen on the frontend so that feedback isn't delayed.

I see the backend providing a simple and robust interface and the frontend providing some more exotic input/formatting options that it can itself parse to the simple backend API.

@kgodey
Copy link
Contributor Author

kgodey commented Nov 24, 2021

@dmos62 I agree with you, but this issue is referring to inferring data types when importing data in bulk (via CSVs, etc.) We don't provide real time feedback in that case and we need to be able to suggest the correct data type.

Please also look at the PR for this issue: #628

@dmos62
Copy link
Contributor

dmos62 commented Nov 25, 2021

I'm surprised I missed that we're talking about bulk import 😳 Needing to parse non-standard time formats on the backend as well as the frontend is complicated, since we want the parser/s to be consistent. It might make sense to consider retreating to parsing only standard formats or finding a way to parse only on the backend or on the frontend.

@mathemancer
Copy link
Contributor

I see your point about the round-trip time, @dmos62 . My biggest concern with the front-end (or service) doing the input parsing is that the DB will have to do input parsing whenever using COPY to bulk load data, and also when the user is using other clients to insert into or select from a table (e.g., psql). This means we'd need to ensure that our front end parsing was identical to that, which is a pain for long-term maintenance. My personal preference would be to start with consistency and correctness (implying using the SQL layer parsing), and then work towards better performance and responsiveness from there. I'd prefer that to restricting to some small subset of the acceptable date / time input styles.

@dmos62
Copy link
Contributor

dmos62 commented Nov 26, 2021

I'm good with doing parsing only on the backend at this point. Only thing to keep in mind is that we might have a hard time porting a non simple parser to frontend (if that's something we ever wanted to do), so format simplicity would still be a good priority.

@kgodey
Copy link
Contributor Author

kgodey commented Nov 26, 2021

I've created a separate issue/discussion to figure out our parsing strategy since we haven't figured out what formats we want to support and how to make backend and frontend parsing consistent: #845. Ideally, someone on the frontend team and someone on the backend team will collaborate on it. cc @pavish @seancolsen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants