tdbc::sqlite3

View Ticket
Login
Ticket Hash: a1e76ad92f66d02fa7eb75b4d0d1231a382677ca
Title: columns metadata method always reports column names in lower case
Status: Closed Type: Code_Defect
Severity: Important Priority: Immediate
Subsystem: tdbc::sqlite3 Resolution: Fixed
Last Modified: 2023-05-25 07:42:03
Version Found In: current
User Comments:
chw added on 2020-05-26 16:03:04:
The connection method "columns" reports the column names of a given table but
the column names are reformatted to lower case with "string tolower".
When data later is retrieved, that reformat on column names is not performed,
i.e. the dict receiving the row data of a result set can have mixed case
column names. Why is this asymmetric behavior? Would it be symmetric, the
column method could easily be used to construct SQL programmatically, e.g.
to build up the bind parameter names for an INSERT statement which then map
to the dict keys of a row of a result set. Or am I missing a subtle detail?

chw added on 2020-05-26 23:27:09:
The attached patch adds a per connection option "-keepcase" which by
default is 0 and if set to non-zero turns on suppression of "string tolower"
when table/column names are processed. Additionally, it uses
"tcl::prefix match" to deal with configuration options.

chw added on 2020-05-27 00:41:10:
tdbcsqlite3.tcl.diff2 is the currently finally latest incarnation of my proposal.

oehhar added on 2023-05-22 13:20:09:

Put into branch [ticket-a1e76ad92f-columns-lowercase] starting with checkin [d9fe8c58e0].

I have no idea what it does. Documentation and tests are missing (what is ok). A wizard may verify. If there are no objections, I will merge to trunk. I suppose, then, the continuous integration will check, if it does not break anything.

Thanks, Christian !

Harald


chw added on 2023-05-22 16:28:52:

Documentation is contained in the patch to tdbc_sqlite.n


oehhar added on 2023-05-24 05:38:27:

After asking on the core list, the following reactions were done:


That looks mostly reasonable (case retention is a valid thing to want). A minor nitpick would be that -keepcase ought to accept anything interpretable as a boolean; it really doesn't need to be an integer and there's no prospect of that being different. I'd normalize on setting with [expr {!!$value}] or something like that. It also ought to be documented as such.

Donal.


This is bikeshedding territory, but fwiw I’m used to seeing this described as “case preserving”; I really don’t want to go on about what colour the shed should be, so may the best name win.

-bch


Christian, do you have any comments ?

I personally would use "bool()" to transform to boolean, but the version by Donal may be more backward compatible.

Harald


chw added on 2023-05-24 06:29:28:
Harald, whatever is best as long as the option is named
"-keepcase" and setting it to "1" yields the described
behavior since I have at least on application in the field
which already uses this feature to perform the task
described in the ticket (generate INSERT statements
programmatically from the column information obtained).

oehhar added on 2023-05-24 09:57:55:

Thank you all for commenting! The result is commit [eeddcaf4e2].

The option name "-preservecase" is IMHO better. We will not be able to change this later on, so this is a critical decission. I am sorry for the incompatibility, but I hope, you will deal with it.

Also, the option is now a boolean and it is normalized to 0/1.

I also added "-strict" to the "string is". By testing, this was not necessary and the error also fired for:

%db configure -timeout ""
expected integer but got ""

But I left it anyway, even as I did not understand it.

Sorry, Harald


oehhar added on 2023-05-24 11:34:09:

Christian commented on the core list:

can I have the synonym "-keepcase" at least for setting the option, please? This would not break my already working and deployed code.

And while at it, if "-preservecase" seems more suitable than "-keepcase" then please think again, whose cases it really preserves. It only works for the methods dealing with database metadata and does nothing for real queries where SQL is directly involved. I.e. to be precise should be called

"-preservecasefordatabasemetadatamethods"

BR, Christian


oehhar added on 2023-05-24 11:57:18:

Thank you, Christian, I appreciate. Lets give others a day to react on this. Your request for an alias is well understood. May I ask some native speakers to speek up?

Example:

% db allrows "create table Test (PersonID int, PastName varchar(255) );"
% db "insert into Test (PersonID, PastName) values (1, 'Myself')"
% ::dbsql::db_process allrows "select * from Test"
{PersonID 1 PastName Myself}

-> remark that the table column names are returned in mixed case

% db columns Test
personid {cid 0 name personid type int notnull 0 pk 0 precision 0 scale 0 nullable 1} pastname {cid 1 name pastname type varchar notnull 0 pk 0 precision 255 scale 0 nullable 1}

-> The column names are reported in small case

% db configure -preservecase true

-> the debate is the option name: -preservecase or -keepcase ?

% ::dbsql::db_process columns Test
PersonID {cid 0 name PersonID type int notnull 0 pk 0 precision 0 scale 0 nullable 1} PastName {cid 1 name PastName type varchar notnull 0 pk 0 precision 255 scale 0 nullable 1}

-> Now, the column names are in original case.

I don't know what idea is behind to report them in small case. For me, this feels more like a bug.

Thank you and take care, Harald


oehhar added on 2023-05-24 13:03:54:

Christian on tcl core:

+1

IMO it is indeed a bug, but an old one, thus the need for a bug compatibility switch to keep things how they're used to be. Another naming possibility therefore might be "-keepbugsindatabasemetadata" with the default value being 1.

Aren't we supposed to eat bugs, Christian


jan.nijtmans added on 2023-05-24 13:26:03:

Since we are bikeshedding anyway ....

Various curren Tcl commands (e.g. regexp or "string compare") have the option "-nocase". Couldn't we simply fix the bug (remove the "tolower" operations everywhere), but allow a "-nocase" or "-nocase true" to make the operation case-insensitive? How many applications would break if the "tolower" behavior was removed totally?


chw added on 2023-05-24 17:29:02:
Once more, you may shed as many bikes, motorcycles, cars, space ships
as you like as long as there's a "-keepcase" synonym for me, I dare to
insist. The ticket is now 2 years old and I've written it for good reason.
I had to solve a real world problem with it. Not to paint some pet objects
with wheels or jet engines in rainbow colors. This ended in a deployed
application, which is alive, kicking and processing production data. It
will be a major effort to upgrade should there be a need for a rename.

oehhar added on 2023-05-25 06:53:14:

Thank you, Christian. It is your ticket. 2 years, nobody cared and now discussion the name is a bit without respect.

In consequence, the name is reverted to -keepcase by commit [130e10fd7e].

Jan, if you want to do more disruptive modifications, this is welcomed. But the major version number should be changed.

I also have code depending on this behaviour. And perhaps, Kevin may report, why this was done. Perhaps, there is a good reason.

Thank you and take care, Harald


jan.nijtmans added on 2023-05-25 07:25:22:

Actually, I consider the "-nocase 1" experiment a failed one, so for now "-keepcase 1" is the best option which is least disruptive.

For tdbc::sqlite3 1.2, I would propose a per-command "-nocase", that's much better than a global know which changes the behavior. But that's low-prio for now


oehhar added on 2023-05-25 07:42:03:

Thanks to all to get a good primilary end. Thanks, Jan, for fixing the test cases.

You guys rock! Harald


Attachments: