Bug 104986 - Firebird: Function EXTRACT doesn't work with WEEKDAY, YEARDAY, MILLISECOND
Summary: Firebird: Function EXTRACT doesn't work with WEEKDAY, YEARDAY, MILLISECOND
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
5.4.0.0.alpha0+
Hardware: x86-64 (AMD64) All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:6.1.0
Keywords:
Depends on:
Blocks: Database-Firebird-Default
  Show dependency treegraph
 
Reported: 2016-12-29 18:27 UTC by robert
Modified: 2018-01-28 08:58 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
Open the query "Extract" and follow the bug-description (6.75 KB, application/vnd.oasis.opendocument.database)
2016-12-29 18:27 UTC, robert
Details

Note You need to log in before you can comment on or make changes to this bug.
Description robert 2016-12-29 18:27:31 UTC
Created attachment 130013 [details]
Open the query "Extract" and follow the bug-description

Firebird offers the function EXTRACT to get the day of the week, as HSQLDB offers this with its own function DAYOFWEEK.
Also the day of the year you could only get with Firebird-function EXTRACT. HSQLDB offers this with DAYOFYEAR.

Open the attached database.
Open the query "Extract" for editing (direct SQL)
Start the query.
There is shown the week of a timestamp in last column. Note: It could only be shown in direct SQL, while (YEAR, MONTH, DAY, HOUR, MINUTE, SECOND) work in GUI, too.
Now set for
EXTRACT(WEEK FROM "StartTimestamp")
EXTRACT(WEEKDAY FROM "StartTimestamp")
EXTRACT(YEARDAY FROM "StartTimestamp")
EXTRACT(MILLISECOND FROM "StartTimestamp")
Last 3 commands end with 
*Token unknown - line 1, column 41
*"WEEKDAY"
...
So there is no possibility to get the WEEKDAY (and also the YEARDAY, MILLISECOND) from the internal Firebird-database.

Tested all with
Version: 5.4.0.0.alpha0+
Build ID: 2a4cd80abcf9e515d1ce3b3a944b573bdc42bff2
CPU Threads: 4; OS Version: Linux 4.1; UI Render: default; VCL: kde4; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2016-12-22_00:18:04
Locale: de-DE (de_DE.UTF-8); Calc: group
Comment 1 m.a.riosv 2016-12-29 22:49:35 UTC
Reproducible.
Version: 5.4.0.0.alpha0+
Build ID: d0288a482a3dc0f50f535565e4c66a95bb140942
CPU Threads: 4; OS Version: Windows 6.19; UI Render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2016-12-26_23:25:18
Locale: es-ES (es_ES); Calc: CL

But it also fails runing the statement from Menu/Tools/SQL
Status box: "caused by 'isc_dsql_prepare'"
Comment 2 Julien Nabet 2016-12-30 11:48:01 UTC
On pc Debian x86-64 with master sources updated today, I could reproduce this.
Taking a look to http://www.firebirdsql.org/file/documentation/reference_manuals/fblangref25-en/html/fblangref25-functions-scalarfuncs.html#fblangref25-funcs-tbl-datediff, it seems that Extract should indeed deal with these params. (but perhaps Firebird 3.0 has some regression about them).

Lionel/Tamas: thought you might be interested in this one.
Comment 3 Lionel Elie Mamane 2016-12-30 13:14:56 UTC
Hmm... That's weird. These all work in a stand-alone firbird 3.0
Comment 4 Lionel Elie Mamane 2016-12-30 13:31:24 UTC
(In reply to Lionel Elie Mamane from comment #3)
> Hmm... That's weird. These all work in a stand-alone firbird 3.0

That might be a bug fixed in Firebird 3.0.1, althought I don't find any trace of it in the firbird jira bug tracker.

Anwyay, upgrading to firebird 3.0.1 would be a good thing anyway... Tamas, you feel like it?
Comment 5 Lionel Elie Mamane 2016-12-30 13:31:51 UTC
(I say that because my stand-alone firebird is 3.0.1 and there it works)
Comment 6 Tamas Bunth 2016-12-30 18:38:57 UTC
(In reply to Lionel Elie Mamane from comment #4)
> (In reply to Lionel Elie Mamane from comment #3)
> > Hmm... That's weird. These all work in a stand-alone firbird 3.0
> 
> That might be a bug fixed in Firebird 3.0.1, althought I don't find any
> trace of it in the firbird jira bug tracker.
> 
> Anwyay, upgrading to firebird 3.0.1 would be a good thing anyway... Tamas,
> you feel like it?

Sure, but I'm quite busy until middle of January.
Comment 7 Julien Nabet 2017-02-19 09:42:23 UTC
Tamas: would you have some time for a patch to upgrade to Firebird 3.0.1 by any chance?
Comment 8 Tamas Bunth 2017-02-19 09:47:41 UTC
(In reply to Julien Nabet from comment #7)
> Tamas: would you have some time for a patch to upgrade to Firebird 3.0.1 by
> any chance?

Sorry, I still don't have time until March.
Comment 9 Gerhard Schaber 2017-04-11 14:04:19 UTC
There is already Firebird 3.0.2.
Comment 10 Tor Lillqvist 2018-01-02 17:04:54 UTC
Suggested fix in https://gerrit.libreoffice.org/#/c/47271/
Comment 11 Lionel Elie Mamane 2018-01-02 17:59:08 UTC
(In reply to Tor Lillqvist from comment #10)
> Suggested fix in https://gerrit.libreoffice.org/#/c/47271/

This change teaches the corresponding keywords to the LibreOffice SQL lexer. The problem described here is that the *Firebird* embedded in LibreOffice refuses these SQL commands (when they are passed bypassing the LibreOffice parser), which is why we were talking about "maybe upgrading Firebird will fix this". As such, I think it is unrelated to this bug seen strictly.

Teaching these keywords to the LibreOffice SQL lexer is useful, since this will allow them to be used in SQL queries that are parsed by LibreOffice (if/when they are supported by the underlying DBMS engine). This will preemptively solve the bug that will immediately be filed after this bug is corrected, namely "doesn't work in LibreOffice-parsed queries". LibreOffice-parser queries unlock several features, like graphical editing of the query, choosing a sort order in the GUI, etc). But we'll also have to put the new tokens in the relevant parser nodes. I'm not sure out of a quick glance if that's non_second_datetime_field (so that it is available in all of datetime_field), or only more specific uses like extract_field. It depends whether these new keywords are valid interval qualifiers or not.
Comment 12 Commit Notification 2018-01-02 19:10:29 UTC
Tor Lillqvist committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=72ff62a13af402d19e6e53b809b29033a985c04a

tdf#104986: Add MILLISECOND, WEEKDAY, and YEARDAY tokens for Firebird

It will be available in 6.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 13 Tor Lillqvist 2018-01-02 19:15:08 UTC
The problem is that Base for some reason seems to uses its SQL lexer and parser to decide whether to pass words in SQL to the database engine as such or quoted. Not sure why it works like that, but there must be some good explanation, I guess.

Before the patch, what it actually passed to Firebird was:

select extract ( "weekday" from "DateTime" ) from "DateTime";

After the patch:

select extract ( weekday from "DateTime" ) from "DateTime";

(Or some such; from my memory.)

Just try the patch...
Comment 14 Tor Lillqvist 2018-01-02 19:15:44 UTC
I did try Firebird 3.0.2, it had no affect on this.
Comment 15 Tor Lillqvist 2018-01-02 19:21:03 UTC
s/affect/effect
Comment 16 Lionel Elie Mamane 2018-01-02 19:29:18 UTC
(In reply to Tor Lillqvist from comment #13)
> The problem is that Base for some reason seems to uses its SQL lexer and
> parser to decide whether to pass words in SQL to the database engine as such
> or quoted.

Urgh. That looks bad. This query is marked as "Run SQL command directly" so I would definitely expect it will take it as a string, and not try _at_ _all_ to parse, much less lex, it.
Comment 17 Lionel Elie Mamane 2018-01-02 19:46:07 UTC
No, it is not LibreOffice itself that "quotes" the WEEKDAY, it is the SDBC Firebird driver. <big sigh of relief>

connectivity::firebird::Connection::prepareStatement is called with:

 SELECT "ID", "StartTimestamp", EXTRACT(WEEKDAY FROM "StartTimestamp") FROM "Table3"

but it quotes it, by calling connectivity::firebird::Connection::transformPreparedStatement

The latter seems to exist only to remove named parameters... Which can be done much more cleanly by setting (hardcoding) the appropriate setting. <sigh>
Comment 18 Lionel Elie Mamane 2018-01-02 20:55:40 UTC
(In reply to Lionel Elie Mamane from comment #17)
> connectivity::firebird::Connection::transformPreparedStatement
> seems to exist only to remove named parameters... Which can be
> done much more cleanly by setting (hardcoding) the appropriate setting.
> <sigh>

Actually, that last part is wrong. The setting is acted upon by the general LibreOffice layer, it is just passed down to the driver, and the driver has to implement the removal of named parameters itself. It seems only the ODBC and the JDBC drivers currently do (althought he MySQL driver seems to know about it), and they, like the current Firebird driver, do it by using the LibreOffice SQL parser to parse the statement, use connectivity::OSQLParseNode::substituteParameterNames to replace named parameters by unnamed ones, and then convert the SQL statement back to string.

There is probably a much more robust way to do it... One only has to grossly lex the SQL, just keeping track of whether on is inside a '-delimited string or a "-delimited identifier. And outside of that, replace any word that starts with ":" with "?".
Comment 19 Lionel Elie Mamane 2018-01-02 22:07:19 UTC
(In reply to Lionel Elie Mamane from comment #18)
> There is probably a much more robust way to do it... One only has to grossly
> lex the SQL, just keeping track of whether on is inside a '-delimited string
> or a "-delimited identifier. And outside of that, replace any word that
> starts with ":" with "?".

It is not quite as easy as I thought, but I think I have a good draft that works for Firebird at least (it wouldn't for PostgreSQL). This assumes that ":" is not allowed in many places in Firebird SQL.

It is at https://gerrit.libreoffice.org/47283
Comment 20 Tor Lillqvist 2018-01-03 09:34:57 UTC
You write: 'No, it is not LibreOffice itself that "quotes" the WEEKDAY, it is the SDBC Firebird driver.'

But isn't "the SDBC Firebird driver" part of LibreOffice? Doesn't that simply mean the code in connectivity/source/drivers/firebird? I would say that that *is* LibreOffice itself.
Comment 21 Lionel Elie Mamane 2018-01-03 09:37:34 UTC
(In reply to Tor Lillqvist from comment #20)
> You write: 'No, it is not LibreOffice itself that "quotes" the WEEKDAY, it
> is the SDBC Firebird driver.'
> 
> But isn't "the SDBC Firebird driver" part of LibreOffice? Doesn't that
> simply mean the code in connectivity/source/drivers/firebird? I would say
> that that *is* LibreOffice itself.

Yes, you are right. I should have written "the DBMS-agnostic layer of LibreOffice".
Comment 22 Commit Notification 2018-01-04 06:51:38 UTC
Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=a29d97e6ddab8ec002ba9827bd5fc874117712e0

tdf#104986 move named parameters substitution into generic layer

It will be available in 6.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 23 Commit Notification 2018-01-07 08:31:36 UTC
Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=27da558d7c1fe45b58d0bcc28b961fabbc5f2f1a

fixup tdf#104986 move named parameters substitution into generic layer

It will be available in 6.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 24 Julien Nabet 2018-01-27 18:28:56 UTC
On pc Debian x86-64 with master sources updated today, I gave a new try.

Here what I got in EXTRACT field with last three requests:
EXTRACT(WEEKDAY FROM "StartTimestamp")
=> 2

EXTRACT(YEARDAY FROM "StartTimestamp")
=> 354

EXTRACT(MILLISECOND FROM "StartTimestamp")
=> 0
Robert: is it ok for you?
Comment 25 robert 2018-01-28 08:28:29 UTC
(In reply to Julien Nabet from comment #24)
> On pc Debian x86-64 with master sources updated today, I gave a new try.
> 
> Here what I got in EXTRACT field with last three requests:
> EXTRACT(WEEKDAY FROM "StartTimestamp")
> => 2
> 
> EXTRACT(YEARDAY FROM "StartTimestamp")
> => 354
> 
> EXTRACT(MILLISECOND FROM "StartTimestamp")
> => 0
> Robert: is it ok for you?

Have tested it with 
Version: 6.1.0.0.alpha0+
Build ID: 6e641f93e837a33c8d4364fdbd88b3d4c52de20c
CPU threads: 4; OS: Linux 4.4; UI render: default; VCL: kde4; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2018-01-27_23:42:42
Locale: de-DE (de_DE.UTF-8); Calc: group threaded

Works as expected in 6.1

I will set this one to RESOLVED - FIXED
Comment 26 Julien Nabet 2018-01-28 08:58:23 UTC
Thank you Robert for your feedback.

Lionel: do you think it worths it to cherry-pick the 3 commits (the Tor's one + the 2 of you) which fixed this one on 6.0 branch or since Firebird is still experimental feature, it doesn't worth it?