Feature/auto casting function arguments #103

jitsedesmet · 2021-08-11T15:43:00Z

This pull request aims to provide an implementation on auto casting functions as talked about in #102 .
This pull request should NOT be merged before the scheme talked about in #102 is added.

There are a few questions to be answered before merging this PR too, those questions should be marked with a TODO.

Edit:
We can close a few issues with this PR: #13, #94 and #103.
When merging we open issues about: cast decimal so we can close #12 , questions surrounding arithmeticWidening, revision of #103 (comment) (I think this issue is mainly for @wschella) and an issue surrounding caching and open world types.
The PR also makes implementation of #87 a lot easier.

coveralls · 2021-08-11T15:50:12Z

Pull Request Test Coverage Report for Build 1138484875

291 of 307 (94.79%) changed or added relevant lines in 10 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+2.4%) to 90.735%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/functions/Helpers.ts	26	27	96.3%
lib/functions/NamedFunctions.ts	3	4	75.0%
lib/functions/RegularFunctions.ts	8	9	88.89%
lib/functions/SpecialFunctions.ts	36	49	73.47%

Totals
Change from base Build 1128445387:	2.4%
Covered Lines:	1389
Relevant Lines:	1499

💛 - Coveralls

lib/functions/OverloadTree.ts

rubensworks

Given the size of this PR, I'm a bit scared of the lack of tests related to this.
We did indeed have tests already that will now cover part of this implementation, but looking at the coveralls output, it seems like coverage is not complete.

Therefore, I would recommend to add more detailed unit tests to the parts of the code you touched. Should be straightforward to do, now that this code is still fresh.

jitsedesmet · 2021-08-12T07:09:33Z

Therefore, I would recommend to add more detailed unit tests to the parts of the code you touched. Should be straightforward to do, now that this code is still fresh.

Do you think about changing the test directory structure at the same time? Like talked about in #34 ? So we can create a dir for unit tests as well?

rubensworks · 2021-08-12T07:11:08Z

Do you think about changing the test directory structure at the same time? Like talked about in #34 ? So we can create a dir for unit tests as well?

Oh, I thought we did that one already 😅

I think that's quite urgent, yes. But I would do it in a separate PR.

wschella · 2021-08-12T07:28:26Z

I can review the PR this afternoon. I think changing the test folder structure can be done & and merged quickly. Then this PR can be rebased on it (since it will be a significant one, and I do really propose fixing everything mentioned in #102 at once here, it's too closely related). We should indeed strive to not decrease test coverage here, but everything should be able to be tested in theory with some entries in a TestTable somewhere.

…implemented things

jitsedesmet · 2021-08-12T11:06:55Z

lib/functions/OverloadTree.ts

+    if (argumentType === TypeURL.XSD_STRING) {
+      this.addPromotedOverload(TypeURL.XSD_ANY_URI, func, arg => string(arg.str()), _argumentTypes);
    }
-    this.subTrees[argumentType]._addOverload(argumentTypes, func);
+    if (argumentType === TypeURL.XSD_FLOAT) {
+      this.addPromotedOverload(TypeURL.XSD_DOUBLE, func, arg =>
+        number((<E.NumericLiteral>arg).typedValue, TypeURL.XSD_DOUBLE), _argumentTypes);
+    }
+    if (argumentType === TypeURL.XSD_DECIMAL) {
+      this.addPromotedOverload(TypeURL.XSD_FLOAT, func, arg =>
+        number((<E.NumericLiteral>arg).typedValue, TypeURL.XSD_FLOAT), _argumentTypes);
+      this.addPromotedOverload(TypeURL.XSD_DOUBLE, func, arg =>
+        number((<E.NumericLiteral>arg).typedValue, TypeURL.XSD_DOUBLE), _argumentTypes);
+    }
+  }
+
+  private addPromotedOverload(typeToPromote: ArgumentType, func: E.SimpleApplication,
+    conversionFunction: (arg: E.TermExpression) => E.TermExpression, argumentTypes: ArgumentType[]): void {
+    if (!this.subTrees[typeToPromote]) {
+      this.subTrees[typeToPromote] = new OverloadTree(this.depth + 1);
+    }
+    this.subTrees[typeToPromote]._addOverload(argumentTypes, args => func([
+      ...args.slice(0, this.depth),
+      conversionFunction(args[this.depth]),
+      ...args.slice(this.depth + 1, args.length),
+    ]));
  }


This code should allow type promotion as talked about in https://www.w3.org/TR/xpath-31/#promotion . At least, how I understand it. If this is wrong please let me know.

At first glance it looks like it could work.
But we'll now for certain once we have tests covering these cases.

Yes, I will add the scheme first and then start writing the tests. With 'wrong' I meant the interpretation of 'promotion'. :)

A special thing to consider here is that subtype substitution and type promotion can work at the same time. There certainly should be tests for that, as I'm not sure the code handles this like it is (but it's also likely that I'm just missing something).

I pretty sure this wouldn't be a problem since promotion is always from a non super type. To another type. And of course like it is now, having a restriction of anyURI provided to a function expecting an xsd_string, the restriction will be substituted to anyUri and will then be promoted to xsd_string.

I'm still convinced we can't really test this. So unless someone can give an example on when this could create a problem I consider this to be done.

I added some documentation so people don't break the system and also a test that first performs substitution and that promotion.

README.md

wschella

Very nice work, very important, and cleans things up nicely.

I also think we we can make it a bit cleaner still: see comments and suggested changes.

lib/Transformation.ts

lib/util/Consts.ts

lib/util/TypeHandling.ts

lib/util/Consts.ts

lib/util/TypeHandling.ts

test/integration/functions/onStrings.test.ts

type-scheme.svg

lib/Transformation.ts

lib/expressions/Term.ts

jitsedesmet · 2021-08-15T09:27:31Z

When merging this PR we should also open 2 new issues.

Is arithmetic widening the right choice? This PR creates a function arithmetic widening this function copy's previous behavior. We should check if this behavior is spec compliant and add links to that part of the spec. Reason: it could be that short + short \ne short. Example: is 240 + 100 still a short? It can not be represented as a short. Should we check this? Or do we need to return a short with value 340 which is not a valid short value.
The requested change by @wschella right here Feature/auto casting function arguments #103 (comment) . Should be checked closer. resolving that issue should also provide an explanation on any choices made there.

…ures.

jitsedesmet · 2021-08-16T13:43:34Z

If we wish to implement https://www.w3.org/TR/xpath-31/#promotion completely, we should make sure we cast the decimal to double or float when promoting decimal. I will leave this for now. I think we should open a new issue adding this (I think small) feature. I feel like we better do this in a new issue because we might need a package for this? I'm not sure though

rubensworks

This stuff wasn't trivial, so really good work!

Before we merge, can you again double-check if all tests (unit, integration, spec, ...) still pass in Comunica?

Could you also list in the main description of this PR, what issues can be closed?

I'll wait a bit for merging until @wschella gives his 👍

jitsedesmet · 2021-08-17T08:34:07Z

Thank you!

Before we merge, can you again double-check if all tests (unit, integration, spec, ...) still pass in Comunica?

I checked the overal tests and ran test, integration and spec in the actor-init-sarql package. All tests succeeded.

I will add the closing issues.

rubensworks · 2021-08-17T08:59:05Z

@wschella Merged this one already. Feel free to open issues in case you still see issues with this.

jitsedesmet mentioned this pull request Aug 11, 2021

Auto casting arguments using OverloadTree #102

Closed

rubensworks reviewed Aug 12, 2021

View reviewed changes

lib/functions/OverloadTree.ts Outdated Show resolved Hide resolved

rubensworks reviewed Aug 12, 2021

View reviewed changes

lib/functions/OverloadTree.ts Outdated Show resolved Hide resolved

rubensworks reviewed Aug 12, 2021

View reviewed changes

jitsedesmet added 11 commits August 12, 2021 11:06

Why do we not have unit tests? Debugging this is gonna hurt

3bc4c27

Resolve changes requested by @rubenworks

2698b0d

Resolve first batch of changes requested by @rubenworks

671b6b6

Remove unnecessary comments

ea6095b

Resolve chages requested by @wschella

e47f57b

First implementation of autocasting feature.

25de8e7

First cleanup before PR.

ae13464

Clear consts file some more

c64b2b0

Use OverloadTree in Builder (lib/functions/Helpers)

718592d

Check on troublesome functions

3b33961

Resolve change requested by @rubenworks and add a warning to wrongly …

8c8e69c

…implemented things

jitsedesmet force-pushed the feature/auto-casting-function-arguments branch from e3bc73f to 8c8e69c Compare August 12, 2021 09:10

jitsedesmet added 4 commits August 12, 2021 11:14

Split OverloadTree file

e681b69

Promotion for anyURI implementetd

e758b8d

clean type promotion

13b5e39

clean _addOverloadImplementation

2cac68e

jitsedesmet commented Aug 12, 2021

View reviewed changes

bugfix & add scheme & update readme

a2265ee

jitsedesmet commented Aug 12, 2021

View reviewed changes

README.md Show resolved Hide resolved

Add some small adidtional test

9819a26

wschella suggested changes Aug 12, 2021

View reviewed changes

jitsedesmet commented Aug 13, 2021

View reviewed changes

lib/Transformation.ts Show resolved Hide resolved

jitsedesmet commented Aug 13, 2021

View reviewed changes

lib/expressions/Term.ts Outdated Show resolved Hide resolved

first batch of changes requested by @wschella

8066455

jitsedesmet added 3 commits August 16, 2021 09:48

Resolving some todo's

bdb0d06

Remove 'type' and 'typeUrl' fields from Literal class

201e735

Resolve todo's added by me - pull request should now contain all feat…

999dc26

…ures.

jitsedesmet and others added 5 commits August 16, 2021 16:00

Add some aditional test dto get (almost) 100% on the parts I (re)wrote

b899a4c

put transformation test back

97292fc

Some additional tests on transformations

99c4be9

Merge branch 'master' into feature/auto-casting-function-arguments

2f8267e

Changes before asking @rubenworks for a second review

864ab5c

rubensworks approved these changes Aug 17, 2021

View reviewed changes

rubensworks merged commit d020e29 into comunica:master Aug 17, 2021

This was referenced Aug 17, 2021

Implement Type Promotion #12

Closed

Adding simple addition benchmark #106

Merged

Clean NumericLiterals specificFormatterCreator #108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/auto casting function arguments #103

Feature/auto casting function arguments #103

jitsedesmet commented Aug 11, 2021 •

edited

coveralls commented Aug 11, 2021 •

edited

rubensworks left a comment

jitsedesmet commented Aug 12, 2021

rubensworks commented Aug 12, 2021

wschella commented Aug 12, 2021

jitsedesmet Aug 12, 2021

rubensworks Aug 12, 2021

jitsedesmet Aug 12, 2021

wschella Aug 13, 2021

jitsedesmet Aug 13, 2021

jitsedesmet Aug 16, 2021

jitsedesmet Aug 16, 2021

wschella left a comment

jitsedesmet commented Aug 15, 2021 •

edited

jitsedesmet commented Aug 16, 2021

rubensworks left a comment

jitsedesmet commented Aug 17, 2021

rubensworks commented Aug 17, 2021

Feature/auto casting function arguments #103

Feature/auto casting function arguments #103

Conversation

jitsedesmet commented Aug 11, 2021 • edited

coveralls commented Aug 11, 2021 • edited

Pull Request Test Coverage Report for Build 1138484875

💛 - Coveralls

rubensworks left a comment

Choose a reason for hiding this comment

jitsedesmet commented Aug 12, 2021

rubensworks commented Aug 12, 2021

wschella commented Aug 12, 2021

jitsedesmet Aug 12, 2021

Choose a reason for hiding this comment

rubensworks Aug 12, 2021

Choose a reason for hiding this comment

jitsedesmet Aug 12, 2021

Choose a reason for hiding this comment

wschella Aug 13, 2021

Choose a reason for hiding this comment

jitsedesmet Aug 13, 2021

Choose a reason for hiding this comment

jitsedesmet Aug 16, 2021

Choose a reason for hiding this comment

jitsedesmet Aug 16, 2021

Choose a reason for hiding this comment

wschella left a comment

Choose a reason for hiding this comment

jitsedesmet commented Aug 15, 2021 • edited

jitsedesmet commented Aug 16, 2021

rubensworks left a comment

Choose a reason for hiding this comment

jitsedesmet commented Aug 17, 2021

rubensworks commented Aug 17, 2021

jitsedesmet commented Aug 11, 2021 •

edited

coveralls commented Aug 11, 2021 •

edited

jitsedesmet commented Aug 15, 2021 •

edited