jupyter-xeus / xeus-sql

Jupyter kernel for SQL databases
https://xeus-sql.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
158 stars 21 forks source link

Change date formatting to a more standard one #39

Closed emredjan closed 3 years ago

emredjan commented 3 years ago

Hi, the formatting of the date / datetime columns in the displayed tables are offloaded to asctime() function as stated here, which displays the dates as a very non-standard way (Www Mmm dd hh:mm:ss yyyy\n),

The display can be done using strftime() instead to display these columns in an ISO compatible format (i.e yyyy-mm-dd hh:mm:ss). My C++ skills is not good enough to come up with a PR for this but I'm assuming it's not a complicated change.

Also, if SOCI supports distinguishing date and datetime columns, time display portion can be stripped from date columns as well.

marimeireles commented 3 years ago

Hey @emredjan, good catch! Thanks for the issue. This one I can do :) Already fixed it, now I'm thinking this might be a breaking change for other people and it'd be better to release a bigger version for this. I need to release a new version of SOCI to be able to release a new version of xeus-sql. Hope to have this stuff done by the end of the day. Thanks again :cherry_blossom:

emredjan commented 3 years ago

Thanks, Maria, but I don't think it worked as you intended. Now date columns come empty with version 0.1.0:

image

marimeireles commented 3 years ago

hum! sorry for that, I'm pretty sure it was working locally! :/ I'll put it on my list to have a look, will try to do it today, thanks for pinging here again.

marimeireles commented 3 years ago

Hi @emredjan, I had a look yesterday and it's working normally for me.

It's also working on the binder instance:

image

Which makes me believe might be some dependency problem. If you can I advise you to be completely inside the conda environment and not allow pip or system packages to interfere with the conda packages. If you're building it from source I advise downloading mamba install cmake compilers.

Maybe a list of what I have in my environment for you to check helps: https://pastebin.com/j5CB3rdY (there's prbb a bunch of things there you don't need)

Please let me know if I can help with something else on how to debug this... What's the OS you're using?

emredjan commented 3 years ago

Thanks @marimeireles for checking.

That command indeed works as intended on my end as well, but selecting a date column from a table shows blank:

image

You can test for yourself:

select datetime('now', 'localtime')

--drop table test1

create table test1
(
    date_col datetime,
    dummy_col nvarchar(10)
)

insert into test1
values
    (datetime('now', 'localtime'), 'dummy')

select * from test1

Or just select * from employees with the example chinook.db:

image

It's reproducible in all these configs:

Although, not even date functions return properly in SQL Server:

image

emredjan commented 3 years ago

Maybe a list of what I have in my environment for you to check helps: https://pastebin.com/j5CB3rdY (there's prbb a bunch of things there you don't need)

That environment shows xeus-sql is still at version 0.0.8? This blank date issue started at 0.1.0. Or is that your development package?

marimeireles commented 3 years ago

That environment shows xeus-sql is still at version 0.0.8? This blank date issue started at 0.1.0. Or is that your development package?

That's true! Sorry I didn't pay attention to this. I thought I was running the build version and not the conda one, and thanks for supplying examples, will look into it now :)

emredjan commented 3 years ago

Also, please do not use the sqlite select datetime('now', 'localtime') function for testing, as the value it returns is a text already in iso format, not a datetime, so it's not relevant to this.

The SQLite datetime() function takes datetimestring & one or more modifier values and returns date and time in YYYY-MM-DD HH:MM:SS format. It is important to note that this function will return the TEXT as an output.

See: https://www.tutlane.com/tutorial/sqlite/sqlite-datetime-function

marimeireles commented 3 years ago

@emredjan uhright Thanks for opening the issue and giving me examples! Should be fixed now! :)

marimeireles commented 3 years ago

Soon a new conda-forge version will come up, it takes a little bit for the changes to take effect. For the next hours if you want to test you must build it from source.

emredjan commented 3 years ago

Soon a new conda-forge version will come up, it takes a little bit for the changes to take effect. For the next hours if you want to test you must build it from source

It's 17:00 on a Friday here, I'm gonna give it a rest until Monday, hopefully the conda-forge package will be up by then :)

emredjan commented 3 years ago

Thanks @marimeireles it works now, with one caveat:

You defined the date formatting as "%Y-%m-%e %H:%M:%S" where %e is the day of the month, with space-padding. However it would be more correct, iso-compatible and consistent with the other parts of the date if you used %d: day of the month, with zero-padding.

Now it looks a bit weird 🤔

image

marimeireles commented 3 years ago

Huh, I didn't notice that, I'll look for a solution for this :) Glad it's working!

emredjan commented 3 years ago

Created #44 for this.