Issue 25 in formulacompiler: ExpressionNodeForSwitch data type is always the same as the one of default value

[formula...@googlecode.com]

Status: Accepted
Owner: vkorenev
CC: peter.arrenbrecht
Labels: Type-Defect Priority-Medium Component-Compiler Milestone-Release1.4

New issue 25 by vkorenev: ExpressionNodeForSwitch data type is always the
same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

See TypeAnnotator.typeOf(ExpressionNodeForSwitch) (TypeAnnotaor.java:279).

This leads to compile-time and run-time errors, such as

org.formulacompiler.compiler.CompilerException$UnsupportedDataType: Cannot
convert from a STRING to a double.

and

java.lang.VerifyError: (class: org/formulacompiler/gen/$Root, method: get$8
signature: (D)D) Expecting to find double on stack

Attachments:
vlookup-tests.patch 15.8 KB

--
You received this message because you are listed in the owner
or CC fields of this issue, or because you starred this issue.
You may adjust your issue notification preferences at:
http://code.google.com/hosting/settings

[formula...@googlecode.com]

Issue 25: ExpressionNodeForSwitch data type is always the same as the one

of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

This issue is now blocking issue 22.
See http://code.google.com/p/formulacompiler/issues/detail?id=22

[formula...@googlecode.com]

Comment #1 on issue 25 by peter.arrenbrecht: ExpressionNodeForSwitch data

type is always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

The attached patch fixes the issue for me and makes the vlookup-tests run
green. Vova,
can you please review and integrate if it's OK?

Attachments:
afc_rev960.patch 4.6 KB

[formula...@googlecode.com]

Updates:
Status: Started

Comment #2 on issue 25 by peter.arrenbrecht: ExpressionNodeForSwitch data

type is always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

(No comment was entered for this change.)

[formula...@googlecode.com]

Comment #3 on issue 25 by vkorenev: ExpressionNodeForSwitch data type is

always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

I've come across some cases which still cause compilation failure.

Attachments:
lookup-ref-tests.patch 101 KB

[formula...@googlecode.com]

Comment #4 on issue 25 by peter.arrenbrecht: ExpressionNodeForSwitch data

type is always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

Good test cases. But they should expect error conditions of type STRING,
not NUMBER.
So their expected values should be:

!STR:NA
!STR:FE
!STR:FE

instead of

!NUM:NA
!NUM:FE
!NUM:FE

This directs the reference test builder to bind the test output cells as
strings, not
as numbers. Then it all works properly.

[formula...@googlecode.com]

Comment #5 on issue 25 by vkorenev: ExpressionNodeForSwitch data type is

always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

Peo, thanks for your notice. The reference tests for H/VLOOKUP pass fine
now.

But there are more errors in other reference tests caused by changed ERROR
type. I
added ad hoc fix for IF (attached patch). But there is (at least) one more
complex
case with ExpressionNodeForFoldDefinition. It can be reproduced by running
the
Aggregators test (COVAR function). Could you please look at it?

Attachments:
type-of-error-in-if.patch 1.4 KB

[formula...@googlecode.com]

Comment #6 on issue 25 by peter.arrenbrecht: ExpressionNodeForSwitch data

type is always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

Vova, I see. Problem are the many occurrences of Function.ERROR in the
generated
rewriter. These are all numeric in type. So we should update the rewrite
rules to
generate typed ERROR instances somehow. Would be ideal if rewrite rules
could specify
their ERROR types explicitly. Can you look into this?

Also, I've attached a slightly improved patch for IF that applies the known
type in
both directions (THEN <-> ELSE).

Attachments:
type-of-error-in-if-2.patch 1.8 KB

[formula...@googlecode.com]

Comment #7 on issue 25 by peter.arrenbrecht: ExpressionNodeForSwitch data

type is always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

The patch is untested, though.

[formula...@googlecode.com]

Comment #8 on issue 25 by vkorenev: ExpressionNodeForSwitch data type is

always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

Ok, I'll look into that.

[formula...@googlecode.com]

Comment #9 on issue 25 by vkorenev: ExpressionNodeForSwitch data type is

always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

Peo, could you look at the attached patch?
Seems that all issues with types in reference tests are fixed.

Attachments:
declare-data-type.patch 10.0 KB

[formula...@googlecode.com]

Comment #10 on issue 25 by vkorenev: ExpressionNodeForSwitch data type is

always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

The above patch extends rewrite.rules syntatx to allow specifying
expression type.
There are two places in rewrite.rules where this is done.

[formula...@googlecode.com]

Comment #11 on issue 25 by vkorenev: ExpressionNodeForSwitch data type is

always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

I extracted common type checking and setting code for IF and SWITCH to a
separate
method in TypeAnnotator. Going to add more test cases to TypeAnnotatorTest.

Attachments:
unify-type-checking.patch 8.8 KB

[formula...@googlecode.com]

Updates:
Status: Fixed
Mergedinto: -
Blocking: -22

Comment #12 on issue 25 by vkorenev: ExpressionNodeForSwitch data type is

always the same as the one of default value
http://code.google.com/p/formulacompiler/issues/detail?id=25

This issue was closed by revision 5c0d2cfef6.