Created attachment 188655 [details] Source database file for copying a table with decimal places Take the two attached databases. Copy the table from "DecimalCopy.odb" to "DecimalCopyTarget.odb". You could add the data to the table in "DecimalCopyTarget.odb" or let the wizard create a new table. No actions needed, so choose "Create" in the wizard. Have a look at the original table and to the result. Field with 2 decimal places: 123.45 will copied to 1.23 (divided by 100) Field with 3 decimal places: 123.456 will copied to 0.123 (divided by 1000) Filed with 4 decimal places: 123.4567 will copied to 0.0123 (divided by 10000) Note: 1. If the data will be pasted to Calc the numbers will be pasted with right values. 2. If the data will be pasted to the same *.odb-file as table source it will also be pasted right.
Created attachment 188656 [details] Target database file to insert data from copied table. Same bug has been reported at https://bugs.documentfoundation.org/show_bug.cgi?id=127618 but this bug has been closed as WORKSFORME. Seems reporter didn't notice he first used Firebird (was the default for some years) and the used HSQLDB (Firebird back to experimental). As we see: Firebird is still experimental …
Bug appears in LO 7.5.5.2 on OpenSUSE, but also in older versions of LO.
I have used the following workaround: copy the table to calc end then copy the calc selection of the table to the new database (append data). The table must exist with the same specifications in the new destination database
Confirming also with Version: 24.2.0.0.alpha0+ (AARCH64) / LibreOffice Community Build ID: 6256d5fe2e7cb1bb002d5fe59527d3a3fbf6963f CPU threads: 8; OS: Mac OS X 13.4; UI render: Skia/Metal; VCL: osx Locale: fr-FR (fr_FR.UTF-8); UI: en-US Calc: threaded
Data loss > upping priority.
One would hope that this isn't a consequence of : https://git.libreoffice.org/core/+/b9a0dc13a02e71dc680ad6b3d1d9f55ae27e3217%5E%21/
@xisco : thoughts ?
(In reply to Alex Thurgood from comment #6) > One would hope that this isn't a consequence of : > > https://git.libreoffice.org/core/+/ > b9a0dc13a02e71dc680ad6b3d1d9f55ae27e3217%5E%21/ Hi Alex, any chance you could revert it and test it ? or bisect it ?
(In reply to Xisco Faulí from comment #8) > (In reply to Alex Thurgood from comment #6) > Hi Alex, > any chance you could revert it and test it ? or bisect it ? Unfortunately, I haven't been able to build environment for LO on Mac for several years now, or even a bibisect environment.
On pc Debian x86-64 with master sources updated today, I could reproduce this. I tried locally by reverting https://git.libreoffice.org/core/+/b9a0dc13a02e71dc680ad6b3d1d9f55ae27e3217%5E%21/, it still fails. I also added a trace to know if LO goes there at this moment, it's not the case.
Trying to debugging a bit: when copying the table, we go in: dbaccess/source/ui/uno/copytablewizard.cxx 1178 case DataType::CHAR: 1179 case DataType::VARCHAR: 1180 case DataType::LONGVARCHAR: 1181 case DataType::DECIMAL: 1182 case DataType::NUMERIC: 1183 aTransfer.transferComplexValue( nSourceColumn, nDestColumn, &XRow::getString, &XParameters::setString ); (see https://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/uno/copytablewizard.cxx?r=9d2355b6#1183) then LO goes in OPreparedStatement::setString See https://opengrok.libreoffice.org/xref/core/connectivity/source/drivers/firebird/PreparedStatement.cxx?r=9227fbab#181 and pbs happen: 181 void SAL_CALL OPreparedStatement::setString(sal_Int32 nParameterIndex, 182 const OUString& sInput) ... 194 OString str = OUStringToOString(sInput , RTL_TEXTENCODING_UTF8 ); ... 198 int dtype = (pVar->sqltype & ~1); // drop flag bit for now ... 200 if (str.getLength() > pVar->sqllen) 201 str = str.copy(0, pVar->sqllen); "sInput" is "123.45" "str" is well converted to "123.45" "dtype" is SQL_LONG (it'll be important next) str.getLength() is equal to 6 => OK but pVar->sqllen = 4 so the str is truncated to "123." not right but whatever, since "dtype" is SQL_LONG, we go there: 242 case SQL_LONG: 243 { 244 sal_Int32 int32Value = sInput.toInt32(); 245 setInt(nParameterIndex, int32Value); 246 break; so we don't use str but "sInput", sInput which contains "123.45" is converted to Int32 so 123. Trying to understand how to fix this, I noticed: https://opengrok.libreoffice.org/xref/core/offapi/com/sun/star/sdbc/XParameters.idl?r=5687eba4#343 ... 334 @param scale 335 for 336 com::sun::star::sdbc::DataType::DECIMAL 337 or 338 com::sun::star::sdbc::DataType::NUMERIC 339 types, this is the number of digits after the decimal point. For all other types, this value will be ignored. 340 @throws SQLException 341 if a database access error occurs. 342 */ 343 void setObjectWithInfo([in]long parameterIndex, 344 [in]any x, [in]long targetSqlType, [in]long scale) 345 raises (SQLException); so "setObjectWithInfo" should be used instead of "setString" in copytablewizard.cxx part. but there's no "getObjectWithInfo" in XRow.idl, just "getObject" in https://opengrok.libreoffice.org/xref/core/offapi/com/sun/star/sdbc/XRow.idl?r=5687eba4#244 Is it sufficient, don't know but I gave a try and then the pb how to adapt the method "transferValue" or "transferComplexValue", 2 template methods from https://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/uno/copytablewizard.cxx?r=9d2355b6#952 In //, I read some info about DECIMAL and NUMERIC here: https://firebirdsql.org/file/documentation/chunk/en/refdocs/fblangref30/fblangref30-datatypes-fixedtypes.html a lot of info here but the point to have in mind is this one: "The form of declaration for fixed-point data, for instance, NUMERIC(p, s), is common to both types. It is important to realise that the s argument in this template is scale, rather than "a count of digits after the decimal point". Understanding the mechanism for storing and retrieving fixed-point data should help to visualise why: for storage, the number is multiplied by 10s (10 to the power of s), converting it to an integer; when read, the integer is converted back." If I don't misunderstand, it means that in Firebird a DECIMAL or NUMERIC with display value "123.45" stores physically "12345" plus some info to indicate it must be divided by 100. Lionel: perhaps you may have some hints to how to fix this? (for devs in general because I expect it won't be easy enough for me :-))
I gave a try with this patch: diff --git a/connectivity/source/drivers/firebird/PreparedStatement.cxx b/connectivity/source/drivers/firebird/PreparedStatement.cxx index 35847d021ea0..529ab8a39cde 100644 --- a/connectivity/source/drivers/firebird/PreparedStatement.cxx +++ b/connectivity/source/drivers/firebird/PreparedStatement.cxx @@ -227,7 +227,7 @@ void SAL_CALL OPreparedStatement::setString(sal_Int32 nParameterIndex, break; case SQL_SHORT: { - sal_Int32 int32Value = sInput.toInt32(); + sal_Int32 int32Value = sInput.replaceAll(".", "").toInt32(); if ( (int32Value < std::numeric_limits<sal_Int16>::min()) || (int32Value > std::numeric_limits<sal_Int16>::max()) ) { @@ -241,13 +241,13 @@ void SAL_CALL OPreparedStatement::setString(sal_Int32 nParameterIndex, } case SQL_LONG: { - sal_Int32 int32Value = sInput.toInt32(); + sal_Int32 int32Value = sInput.replaceAll(".", "").toInt32(); setInt(nParameterIndex, int32Value); break; } case SQL_INT64: { - sal_Int64 int64Value = sInput.toInt64(); + sal_Int64 int64Value = sInput.replaceAll(".", "").toInt64(); setLong(nParameterIndex, int64Value); break; } Remarks: 1) the "replaceAll" is required for both SQL_LONG and SQL_INT64, so concerning SQL_SHORT it was more to be the same way 2) It was OK with French UI so I supposed it's not useful to test if decimal separator was ".", "," or whatever. Indeed it seemed to me only the internal representation of the value (which I expect always use ".") must be used. Result: non decimal part is ok but not the decimal part, we lose some precision Conclusion: not the right thing to do :-(
(In reply to Julien Nabet from comment #11) > "sInput" is "123.45" > "str" is well converted to "123.45" > "dtype" is SQL_LONG (it'll be important next) Well, the code seems to (mistakenly?) believe the value should be set into a (long) integer field. > str.getLength() is equal to 6 => OK > but > pVar->sqllen = 4 so the str is truncated to "123." Well, it smells like this comes from the (wrong?) value of dtype. > not right but whatever, since "dtype" is SQL_LONG, we go there: > 242 case SQL_LONG: > 243 { > 244 sal_Int32 int32Value = sInput.toInt32(); > 245 setInt(nParameterIndex, int32Value); > 246 break; > so we don't use str but "sInput", sInput which contains "123.45" is > converted to Int32 so 123. Well, yes, since according to dtype, we need an integer, so we get an integer out of the data. > In //, I read some info about DECIMAL and NUMERIC here: > https://firebirdsql.org/file/documentation/chunk/en/refdocs/fblangref30/ > fblangref30-datatypes-fixedtypes.html > > a lot of info here but the point to have in mind is this one: > "The form of declaration for fixed-point data, for instance, NUMERIC(p, s), > is common to both types. It is important to realise that the s argument in > this template is scale, rather than "a count of digits after the decimal > point". Understanding the mechanism for storing and retrieving fixed-point > data should help to visualise why: for storage, the number is multiplied by > 10s (10 to the power of s), converting it to an integer; when read, the > integer is converted back." > If I don't misunderstand, it means that in Firebird a DECIMAL or NUMERIC > with display value "123.45" stores physically "12345" plus some info to > indicate it must be divided by 100. Well, OK, but that is an internal Firebird implementation decision and detail. No code outside of connectivity/source/drivers/firebird should need to know that or be aware of that.
(In reply to Lionel Elie Mamane from comment #13) > Well, OK, but that is an internal Firebird implementation decision and > detail. No code outside of connectivity/source/drivers/firebird should need > to know that or be aware of that. Ah, but we are in the Firebird-specific code. I'm biting... Reading the "InterBase 6" API GUIDE pages 91 to 93, it seems indeed that in Firefox, in the C-level API, NUMERIC and DECIMAL give an "sqltype" of an integer... So we need to "understand" whether that is really an integer or the integer storage for a NUMERIC or DECIMAL, but I don't immediately see how one determines that.
(In reply to Lionel Elie Mamane from comment #14) > (In reply to Lionel Elie Mamane from comment #13) > >... > Ah, but we are in the Firebird-specific code. I was responding when I saw this second message, it's indeed the pb. > > I'm biting... Reading the "InterBase 6" API GUIDE pages 91 to 93, it seems > indeed that in Firefox, in the C-level API, NUMERIC and DECIMAL give an > "sqltype" of an integer... So we need to "understand" whether that is really > an integer or the integer storage for a NUMERIC or DECIMAL, but I don't > immediately see how one determines that. In the doc page I quoted (https://firebirdsql.org/file/documentation/chunk/en/refdocs/fblangref30/fblangref30-datatypes-fixedtypes.html), take a look at: "Table 3.3.1 Method of Physical Storage for Real Numbers". It's an array so can't copy this directly in the bugtracker without losing the whole formatting.
(In reply to Lionel Elie Mamane from comment #14) > So we need to "understand" whether that is really > an integer or the integer storage for a NUMERIC or DECIMAL, but I don't > immediately see how one determines that. Hmm... Try something like that: case SQL_LONG: { short sqlscale = pVar->sqlscale; // move the decimal in the string by -sqlscale // and AFTER THAT take the integer part of it // e.g. if the string is "123.456789" and sqlscale is -1 // then int32Value should be "1234" // if the string is "123.456789" and sqlscale is -2 // then int32Value should be.... maybe "12345" // or maybe "12346" because we need to use rounding, not truncation? // yes, I think do rounding, not truncation setInt(nParameterIndex, int32Value); break; } and do the same for the other cases: SQL_SHORT, SQL_INT64, etc. Additionally this code is fundamentally wrong, at least placed where it is. 200 if (str.getLength() > pVar->sqllen) 201 str = str.copy(0, pVar->sqllen); It is not linked to "believing we need an integer value", it is just a coincidence; an SQL_LONG takes four bytes AND IN THIS PARTICULAR EXAMPLE, four CHARACTERS OF THE STRING takes the integer value plus the dot, BUT THAT'S JUST A COINCIDENCE. This code should be only in the codepath of the cases SQL_VARYING, SQL_TEXT, SQL_BLOB, and NOT in the cases SQL_INT/LONG/FLOAT/etc. It would be nice to have a unittest for all that. It would be pretty simple, just have a firebird database with columns of different types (VARCHAR, NUMERIC, DECIMAL, INTEGER, FLOAT, ...) and try to set values of these columns by setString(), then read the value from the database and check it is the correct one depending on the string.
(In reply to Lionel Elie Mamane from comment #16) > (In reply to Lionel Elie Mamane from comment #14) > > > So we need to "understand" whether that is really > > an integer or the integer storage for a NUMERIC or DECIMAL, but I don't > > immediately see how one determines that. > > Hmm... Try something like that: > >... Ok I'll try this (rather tomorrow since it's getting a bit late for me) > Additionally this code is fundamentally wrong, at least placed where it is. > > 200 if (str.getLength() > pVar->sqllen) > 201 str = str.copy(0, pVar->sqllen); > > It is not linked to "believing we need an integer value", it is just a > coincidence; an SQL_LONG takes four bytes AND IN THIS PARTICULAR EXAMPLE, > four CHARACTERS OF THE STRING takes the integer value plus the dot, BUT > THAT'S JUST A COINCIDENCE. This code should be only in the codepath of the > cases SQL_VARYING, SQL_TEXT, SQL_BLOB, and NOT in the cases > SQL_INT/LONG/FLOAT/etc. > ... I had noticed this too and had begun to change this. However, I had removed it in my first attempt to reduce noise. In the same way, OString str = OUStringToOString(sInput , RTL_TEXTENCODING_UTF8 ); is only useful for SQL_VARYING and SQL_TEXT parts. and finally just a nitpick, I thought about adding brackets for SQL_TEXT and SQL_BLOB (just wonder why only these don't have them). Thank you for all the feedback! :-)
I gave a try with LONG part only for the moment with this patch: diff --git a/connectivity/source/drivers/firebird/PreparedStatement.cxx b/connectivity/source/drivers/firebird/PreparedStatement.cxx index 35847d021ea0..53d1596a2608 100644 --- a/connectivity/source/drivers/firebird/PreparedStatement.cxx +++ b/connectivity/source/drivers/firebird/PreparedStatement.cxx @@ -241,7 +241,19 @@ void SAL_CALL OPreparedStatement::setString(sal_Int32 nParameterIndex, } case SQL_LONG: { - sal_Int32 int32Value = sInput.toInt32(); + short sqlscale = pVar->sqlscale; + + OUString str1 = sInput; + if (sqlscale) + { + sal_Int32 posSep = str1.indexOf(u'.'); + if (posSep != -1) + { + OUString tmp = sInput.replaceAll(".", ""); + str1 = tmp.subView(0, posSep - sqlscale); + } + } + sal_Int32 int32Value = str1.toInt32(); setInt(nParameterIndex, int32Value); break; } I got the same result as first patch. I mean, - if sInput = 123.456 then sqlscale = 3 so the value is 123456 - if sInput = 2345.16 then sqlscale = 2 so the value is 234516 so I got the same as just removing "."
(In reply to Julien Nabet from comment #18) > - if sInput = 123.456 then sqlscale = 3 so the value is 123456 > - if sInput = 2345.16 then sqlscale = 2 so the value is 234516 > so I got the same as just removing "." This may be true in this specific scenario of copying from a Firebird table to a Firebird table, but setString() is a generic API call, and should handle correctly any sane string... Also e.g. setString("123.456") when sqlscale=0 or sqlscale=2 or sqlscale=5.
Lionel: here's a WIP patch https://gerrit.libreoffice.org/c/core/+/156048
(In reply to Julien Nabet from comment #20) > Lionel: here's a WIP patch https://gerrit.libreoffice.org/c/core/+/156048 I abandoned the patch, it's a dead end. I got no more idea here.
Reproduced in: Version: 7.0.6.2 Build ID: 144abb84a525d8e30c9dbbefa69cbbf2d8d4ae3b CPU threads: 8; OS: Linux 5.15; UI render: default; VCL: gtk3 Locale: en-AU (en_AU.UTF-8); UI: en-US Calc: threaded In 6.0 (experimental features turned on), 6.1, 6.2, 6.3 and 6.4, pasting would give an error and result in an empty table. So I don't see it as a regression, I don't think such a paste ever worked.
(In reply to Julien Nabet from comment #21) > (In reply to Julien Nabet from comment #20) >> Lionel: here's a WIP patch https://gerrit.libreoffice.org/c/core/+/156048 > I abandoned the patch, it's a dead end. The patch just needs some more work, but it goes in the right direction. I'll try to work on it when I have some time (may take some weeks).
*** Bug 157886 has been marked as a duplicate of this bug. ***