Bug Hunting Session
Bug 31296 - Support sheet local defined names
Summary: Support sheet local defined names
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: x86 (IA32) All
: high enhancement
Assignee: Kohei Yoshida
URL:
Whiteboard: target:3.4
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-01 08:50 UTC by Michel Rudelle
Modified: 2011-03-15 21:09 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments
Original Excel spreadsheet (664.00 KB, application/vnd.ms-excel)
2010-11-01 08:50 UTC, Michel Rudelle
Details
Calc spreadsheet with invalid names (312.11 KB, application/vnd.oasis.opendocument.spreadsheet)
2010-11-01 08:53 UTC, Michel Rudelle
Details
name manager in Excel 2007 (20.98 KB, image/png)
2010-11-02 14:28 UTC, Kohei Yoshida
Details
This is what I see in CI.P11 (106.64 KB, image/png)
2010-11-02 18:25 UTC, Kohei Yoshida
Details
wrong result of importation (62.00 KB, application/vnd.ms-excel)
2010-11-08 13:21 UTC, Michel Rudelle
Details
good result of importation (16.50 KB, application/vnd.ms-excel)
2010-11-08 13:23 UTC, Michel Rudelle
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michel Rudelle 2010-11-01 08:50:34 UTC
Created attachment 39954 [details]
Original Excel spreadsheet

The problem is observed during the import of an Excel spreadsheet file:
The same area of cells can be redefined with several names, these various names are generated with the original name followed by an index.
With LibO (as well as OOo 3.3 RC3) this index is separated from the name by a space:
Example: 
    profil_section 2
    profil_section 3
This form is not correct.
This is a regression because names are correctly generated with version 3.2 (an underscore is used instead of a space).
Comment 1 Michel Rudelle 2010-11-01 08:53:18 UTC
Created attachment 39955 [details]
Calc spreadsheet with invalid names

Result of the import of the Excel file
Comment 2 sophie 2010-11-01 11:42:29 UTC
Confirmed under Ubuntu too. Reassigned to Kohei - Thanks - Sophie
Comment 3 andréb 2010-11-01 17:39:11 UTC
Just took a quick look, tracing a reference.

There is a "missing operator" error at tds:p11.
The formula is

"=SI(OU(G11<$H$7;G11>$J$7);"";ARRONDI.SUP(($O11+K11^2)^0,5;2))"

I don't have Excel (or Microsoft windows), so I don't know what it should be.
The same error message appears in neighbouring cells.

This formula appears to have correct syntax - so it is an error by LibreOffice in resolving the formula.

1) if either G11 < H7 or G11 > J7 then should give a blank (or null)

2) otherwise should give square root of (O11 + K11^2), rounded up, to 2 decimal places

Unless I'm missing something ...
Comment 4 andréb 2010-11-01 18:17:56 UTC
further note :

My LibreOffice is in French.
In English, the formula giving the "missing operator" error message
 (probably) would be :

"=IF(OR(G11<$H$7;G11>$J$7);"";ROUND.UP(($O11+K11^2)^0,5;2))"

Also, I'm running Mandriva Linux

- André
Comment 5 Michel Rudelle 2010-11-02 02:27:05 UTC
(In reply to comment #3)
> Just took a quick look, tracing a reference.
> 
> There is a "missing operator" error at tds:p11.
> The formula is
> 
> "=SI(OU(G11<$H$7;G11>$J$7);"";ARRONDI.SUP(($O11+K11^2)^0,5;2))"
> 
> I don't have Excel (or Microsoft windows), so I don't know what it should be.
> The same error message appears in neighbouring cells.
> 
> This formula appears to have correct syntax - so it is an error by LibreOffice
> in resolving the formula.
> 
> 1) if either G11 < H7 or G11 > J7 then should give a blank (or null)
> 
> 2) otherwise should give square root of (O11 + K11^2), rounded up, to 2 decimal
> places
> 
> Unless I'm missing something ...

The formula you show is in sheet “CI” and not “tds”
You don't need to use Excel to see the problem: look at the evaluation of cell P11 in sheet “CI”:
P11 <= G11 <= F11 <= H27 <= C27:G27
(P11 <= G11 means that the formula in P11 use the evaluation of formula in G11)
Formula in area C27:G27 refers to the named area “profil_section 3” (with a space between section and 3), this is an invalid name and the first occurrence of Err:509 in the series of cells.
With Excel and OOo 3.2 the area is named “profil_section”.
Comment 6 Kohei Yoshida 2010-11-02 09:04:27 UTC
What I don't understand is, while Excel shows only profil_section name, Calc shows

profil_section
profil_section 2
profil_section 3

and only the last entry contains valid reference.  There is something definitely funny going on here during the import of range names.
Comment 7 Kohei Yoshida 2010-11-02 11:35:53 UTC
Ok.  dr changed something here.  The bug number points to Oracle's internal bug tracker (#163146#), so we'll never know what his motivation really was.
Comment 8 Kohei Yoshida 2010-11-02 14:22:59 UTC
Ah, dr's change is responsible for the underscore to space name changes, but that change is *not* responsible for the breakage of the formulas.  That's just one harmless change that happened to be visible.

The real issue here is this: the test document contains lots of local defined names i.e. names that are available only at sheet scope, but Calc only supports global names at the moment.  So, this bug is just a manifestation of that limitation.

The real fix is to properly support sheet-local names so that we could cleanly import Excel's sheet-local names.  But that's for future versions.
Comment 9 Kohei Yoshida 2010-11-02 14:25:16 UTC
IOW, the fact that the formula worked at all in 3.2 is just a coincidence; it shouldn't have worked.
Comment 10 Kohei Yoshida 2010-11-02 14:28:01 UTC
Created attachment 39996 [details]
name manager in Excel 2007

Excel 2007 has a better UI for defined names, which clearly shows multiple definitions of profil_section etc, and their associated scopes.  The Workbook scope means the name is available globally to all sheets, otherwise the sheet name where the name is available is shown.
Comment 11 Kohei Yoshida 2010-11-02 14:30:05 UTC
Changing the title accordingly.  Technically this is an enhancement for better Excel interop.
Comment 12 Kohei Yoshida 2010-11-02 18:21:49 UTC
Actually, this may have nothing to do with sheet-local name import.  Changing the title back.

Michel, I don't see any Err:509 here.  Can you tell me exactly in what cells you see Err:509?

I'm using the latest development build, so your problem may have already been fixed.  But I do need to double-check that.
Comment 13 Kohei Yoshida 2010-11-02 18:25:40 UTC
Created attachment 40001 [details]
This is what I see in CI.P11

This is what I see in CI.P11, using the latest development build.
Comment 14 Kohei Yoshida 2010-11-02 18:28:05 UTC
I still believe that we need to import sheet-local names correctly by implementing support for them in calc core, but this bug doesn't seem to be caused by that limitation.
Comment 15 andréb 2010-11-02 19:24:19 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > Just took a quick look, tracing a reference.
> > 
> > ...
> > 
> > Unless I'm missing something ...
> 
> The formula you show is in sheet “CI” and not “tds”
> You don't need to use Excel to see the problem: look at the evaluation of cell
> P11 in sheet “CI”:
> P11 <= G11 <= F11 <= H27 <= C27:G27
> (P11 <= G11 means that the formula in P11 use the evaluation of formula in G11)
> Formula in area C27:G27 refers to the named area “profil_section 3” (with a
> space between section and 3), this is an invalid name and the first occurrence
> of Err:509 in the series of cells.
> With Excel and OOo 3.2 the area is named “profil_section”.

Ok, it is in sheet "CI".

I see now.
It is actually
H27 <= f(C27 to G27) =RECHERCHEV($B27;profil_section 3;offset_column)

So it's just carrying forward the Err:509 "missing operator" error.

Maybe this worked (or at least seemed to work) in OOo 3.2 because the trailing " 3" was ignored ?
Comment 16 andréb 2010-11-02 19:33:25 UTC
(In reply to comment #13)
> Created an attachment (id=40001) [details]
> This is what I see in CI.P11
> 
> This is what I see in CI.P11, using the latest development build.

I downloaded the spreadsheet showing the errors.
Everywhere in your screenshot you show a %, there was "Err :509".
So it is evidently now fixed.
Comment 17 Kohei Yoshida 2010-11-02 19:49:59 UTC
Thanks.  I'll happily mark this fixed. :-)
Comment 18 Michel Rudelle 2010-11-03 03:57:54 UTC
(In reply to comment #12)
> ...
> 
> Michel, I don't see any Err:509 here.  Can you tell me exactly in what cells
> you see Err:509?
> 
> I'm using the latest development build, so your problem may have already been
> fixed.  But I do need to double-check that.

Checked with libreoffice-build 3.2.99.2

I obtain two different results according to the way of proceeding:
1) In one step:
	open the Excel file and save it with LibO in ODS format
	= > error 509
2) In two steps:
	a) open the Excel file and save it with LibO in XLS format, then close the file (closure is compulsory !)
	b) open the xls file created with LibO and save it in ODS format
	 = > correct !
There is a solution which works, it is less urgent, but isn't it surprising ?
What do you think of that ?
Comment 19 Kohei Yoshida 2010-11-03 07:09:51 UTC
(In reply to comment #18)

> I obtain two different results according to the way of proceeding:
> 1) In one step:
>     open the Excel file and save it with LibO in ODS format
>     = > error 509

Reproduced.  Not good.

> 2) In two steps:
>     a) open the Excel file and save it with LibO in XLS format, then close the
> file (closure is compulsory !)
>     b) open the xls file created with LibO and save it in ODS format
>      = > correct !
> There is a solution which works, it is less urgent, but isn't it surprising ?
> What do you think of that ?

Based on what we see, we may be using something that Excel supports (hence xls import / export retains fidelity) but ODF doesn't.

Let me look into that a bit.  Meanwhile, if you could also reduce the size of your test document that can still reproduce the bug, I will be very grateful. :-)
Comment 20 Kohei Yoshida 2010-11-03 08:19:16 UTC
Ok.  I'm pretty sure that the Err:509 is caused by an incorrect parsing of the formula string involving a name with whitespace.

Now, fixing this will not be easy, and anything we do other than supporting sheet-local range names would be a hack.  So, I still think the correct solution is to implement sheet-local range names even though that will require more engineering resources.

Let me re-open this.
Comment 21 Kohei Yoshida 2010-11-03 08:23:16 UTC
Changing the title once again.
Comment 22 Michel Rudelle 2010-11-03 09:02:10 UTC
(In reply to comment #19)
>  Meanwhile, if you could also reduce the size of
> your test document that can still reproduce the bug, I will be very grateful.
> :-)

I'm going to try, but not before 2 or 3 days, that's ok ?
Comment 23 Kohei Yoshida 2010-11-03 09:37:36 UTC
(In reply to comment #22)

> I'm going to try, but not before 2 or 3 days, that's ok ?

Yup, no rush.  Whenever you have time. :-)
Comment 24 Michel Rudelle 2010-11-08 13:21:34 UTC
Created attachment 40126 [details]
wrong result of importation

Here are 2 files which seem identical (I use Excel 2003):
 - “reduced_file.xls” is obtained by reducing my initial file
 - “new_file.xls” is a file created by reproducing manually “reduced_file.xls”.

They look identical, but the size is different, and the result of importation (in one step) is wrong with “reduced_file” and correct with “new_file”.

Then, it is a bug of Calc or Excel ?
There is something which is not visible in reduced_file and raises problem.
Comment 25 Michel Rudelle 2010-11-08 13:23:39 UTC
Created attachment 40127 [details]
good result of importation
Comment 26 Kohei Yoshida 2011-01-04 16:12:24 UTC
Looks like the next significant release will be 3.4, not 3.3.1.  Resetting the target accordingly.
Comment 27 Kohei Yoshida 2011-03-09 12:55:42 UTC
I'm working on this on master (for 3.4).  The reduced file now calculates correctly.  The original file causes Calc to crash now.  Looking into that. :-/
Comment 28 Kohei Yoshida 2011-03-09 15:33:31 UTC
(In reply to comment #27)
> I'm working on this on master (for 3.4).  The reduced file now calculates
> correctly.  The original file causes Calc to crash now.  Looking into that. :-/

Ah!  The crash has nothing to do with my change, but someone else's. ;-)

Anyway, the original file now loads and I don't see any more Err:50X's.  I still have to work on ODS import and export and the UI part, but I'm making progress.
Comment 29 Kohei Yoshida 2011-03-11 13:18:45 UTC
The core change as well as ODS XLS import export are done & are merged into master.  All that remains is the UI change.
Comment 30 Kohei Yoshida 2011-03-15 21:09:43 UTC
All done.  This just landed on master, in time for 3.4.