pow() does not throw error for extra or incorrect arguments like quotient() does

[thadguidry]

To Reproduce

Steps to reproduce the behavior:

1. Open Expression Editor on existing project
2. On any column use Custom Text Transform with value.pow(2,8)
3. Notice the preview result of null
4. Change the expression to value.quotient(2,8)
5. Notice the preview result of Error: quotient expects two numbers

Current Results

null is shown in preview result, instead of an error (since value is passed as one of the arguments with additional 2 arguments (2,8) supplied)

Expected Behavior

pow() should check arguments like quotient() does and show similar error.

Screenshots

Versions

• Operating System: Windows 10
• Browser Version: Firefox latest
• JRE or JDK Version: JDK8
• OpenRefine: openrefine-3.4-beta-458-g109aa78

Datasets

Additional context

[chetan-v]

I did something for it. @thadguidry please check.

[thadguidry]

@tfmorris or @wetneb will check (they are 2 of our core developers) as I'm staying out of active development. But I will be happy to test once merged.

[wetneb]

What do you intend to compute with value.pow(2,8)?
If you want to compute 2*2*2*2*2*2*2*2 (aka 256), then you should just use pow(2,8), not value.pow(2, 8). Same for quotient.

[thadguidry]

@wetneb Oh you are absolutely right! Hmm, we should give warnings then. Perhaps I should change this issue title and comment? Or close this and open a new one might be best.

[chetan-v]

Hi @thadguidry
I use only pow(,8) and they give null.
I see the function that only return null not any exception.
main/src/com/google/refine/expr/functions/math/Pow.java
Which is in this file.
I add that line from function of call of quotient.java file that show this Error: quotient expects two numbers .

[tfmorris]

What do you intend to compute with value.pow(2,8)?
If you want to compute 2*2*2*2*2*2*2*2 (aka 256), then you should just use pow(2,8), not value.pow(2, 8). Same for quotient.

Yes, it's an error, but I think the point of the original bug report was that the error handling for the two cases should be equivalent. They should either both return error or both return null. I have a strong preference for the former.

This isn't limited to pow(). All of these functions are exempt from the test for too many arguments:

OpenRefine/main/tests/server/src/com/google/refine/grel/FunctionTests.java

Lines 133 to 137 in c8a721a

	// Not sure which, if any, of these are intended, but fixing them may break existing scripts
	Set<String> returnsNull = new HashSet<>(Arrays.asList("chomp", "contains", "coalesce", "escape", "unescape",
	"exp", "fingerprint", "get", "now", "parseJson", "partition", "pow", "rpartition",
	"slice", "substring", // synonyms for Slice
	"unicode", "unicodeType"

"chomp", "contains", "coalesce", "escape", "unescape", "exp", "fingerprint", "get", "now", "parseJson", "partition", "pow", "rpartition", "slice", "substring", "unicode", "unicodeType"

[elroykanye]

This isn't limited to pow(). All of these functions are exempt from the test for too many arguments:

OpenRefine/main/tests/server/src/com/google/refine/grel/FunctionTests.java

Lines 133 to 137 in c8a721a

	// Not sure which, if any, of these are intended, but fixing them may break existing scripts
	Set<String> returnsNull = new HashSet<>(Arrays.asList("chomp", "contains", "coalesce", "escape", "unescape",
	"exp", "fingerprint", "get", "now", "parseJson", "partition", "pow", "rpartition",
	"slice", "substring", // synonyms for Slice
	"unicode", "unicodeType"

"chomp", "contains", "coalesce", "escape", "unescape", "exp", "fingerprint", "get", "now", "parseJson", "partition", "pow", "rpartition", "slice", "substring", "unicode", "unicodeType"

Would we then have to modify every single one of these?

[antoine2711]

Would we then have to modify every single one of these?

Yes @elroykanye. It's the same pattern to use to check.
(It's possible some might have been fixed with other issues, but unlikely.)

Regards,
Antoine

[elroykanye]

Alright then, I will go through each and enforce the pattern of returning an EvalError if there are no objections.

[elroykanye]

OpenRefine/main/tests/server/src/com/google/refine/grel/FunctionTests.java

Lines 132 to 134 in c8a721a

	void testTooManyArgs() {
	// Not sure which, if any, of these are intended, but fixing them may break existing scripts
	Set<String> returnsNull = new HashSet<>(Arrays.asList("chomp", "contains", "coalesce", "escape", "unescape",

I am really concerned with the comment at L131. Will changing the returns of these affect the tests that have already been prepared?
I am sure that will happen. If that is the case, is it alright to modify the tests?

[antoine2711]

Will changing the returns of these affect the tests that have already been prepared? I am sure that will happen. If that is the case, is it alright to modify the tests?

@elroykanye : yes, you will have to adapt some test cases, write new ones, and be careful… ;-)

Regards, Antoine

[elroykanye]

Okay, thanks 😁

[antoine2711]

@elebitzero: there seams to be a misunderstanding here. @elroykanye was assigned the issue, and I see that you submitted a Pull Request. Before creating a PR, please make sure no one is already assigned.

In the OpenRefine community, we value your time and efforts, and if 2 persons works on the same issue at the same time, we fear it could be an inappropriate use of your time, making one of you two working for nothing.

That being said, I appreciate your involments — both of you — thanks you for your contributions.

Regards, Antoine

[elroykanye]

Thanks @antoine2711 .
That's true, I would like to be informed or notified when working on issue to avoid clashes. I can always leave room for someone else to offer some help in it.
@elebitzero , thanks for your contribution. I believe it will be proper that you notify when wishing to work on an assigned issue 😊 it will make our contributions frictionless and free.

[elebitzero]

@antoine2711 @elroykanye, oops, I'm just seeing these recent comments now. It looks like I picked up this issue at the same time @elroykanye started looking into it. I should have commented on the issue first.

I fixed the issue where null.pow(2) returns null instead of an error like all the other math functions. Also, I found that null.exp() returned null instead of an error. So the issue fixed by my PR is that pow() and exp() now return Error instead of null for null arguments, so they are now consistent with the other math functions.

I also added a new test testNullArgsMath to check that the math functions return an error with null argument(s).

Note: @thadguidry mentioned value.pow(2,8) which is not valid because pow() expects two numbers. In this case, my fix will display an error instead of null.

[elebitzero]

"chomp", "contains", "coalesce", "escape", "unescape", "exp", "fingerprint", "get", "now", "parseJson", "partition", "pow", "rpartition", "slice", "substring", "unicode", "unicodeType"

Would we then have to modify every single one of these?

@elroykanye, my PR removes "exp" and "pow" from the returnsNull set in the testTooManyArgs test. They now return error for incorrect number of arguments. The other functions are string functions; I would create a separate issue for those.

[antoine2711]

@antoine2711 @elroykanye, oops, I'm just seeing these recent comments now. It looks like I picked up this issue at the same time @elroykanye started looking into it. I should have commented on the issue first.

Actually, @elebitzero, the key here is the person « Assigned » to the issue. The first who put his name there is the one in charge, so to say.

Anyway, since you and @elroykanye are at a good profeciency level, I guess you will be able to work together on this issue. ;-)

Regards, Antoine

[elroykanye]

@elebitzero we can definitely work together so this can be resolved 😃😁
I'll go through your PR and select out what is not included so I can launch my help too.