You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "mike-mcgann (via GitHub)" <gi...@apache.org> on 2023/02/15 21:22:20 UTC
[GitHub] [daffodil] mike-mcgann opened a new pull request, #966: Improve errors with contentLength/valueLength
mike-mcgann opened a new pull request, #966:
URL: https://github.com/apache/daffodil/pull/966
This is a draft.
The original ticket was to fix unexpected results with contentLength/valueLength when they are used on an array. That doesn't seem to be an issue anymore as it currently emits an SDE but the message references the parent node and not the array. I put in a Scalar type as recommended in the ticket discussion and that now creates an error message with the correct node.
I added an additional test to ensure there is an error when a string literal is used as the first argument. That causes a ClassCastException. I wasn't sure how to restrict that from happening using the type system and I just hacked in a type check to instead throw an SDE. I need some suggestions on how to implement this fix properly.
[DAFFODIL-1651](https://issues.apache.org/jira/browse/DAFFODIL-1651)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1115788316
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml:
##########
@@ -459,6 +459,37 @@
</xs:complexType>
</xs:element>
+ <xs:element name="valueLength14">
+ <xs:complexType>
+ <xs:sequence dfdl:separator="%WSP;">
+ <xs:element name="first" type="xs:string" dfdl:lengthKind="delimited"/>
+ <xs:element name="other" type="xs:string" dfdl:lengthKind="delimited" maxOccurs="unbounded"/>
+ <xs:element name="len" type="xs:unsignedInt" dfdl:inputValueCalc='{dfdl:valueLength(../ex:other, "bytes")}'/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="valueLength15" type="xs:unsignedInt" dfdl:inputValueCalc='{dfdl:valueLength("three", "bytes")}'/>
+
+ <xs:element name="valueLength16">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="e1">
Review Comment:
Yeah, unbounded makes for a better test. I've added that change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1112034874
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,19 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
- val arg = args(0).asInstanceOf[PathExpression]
+ val arg = args(0) match {
+ case arg: PathExpression => arg
+ case _ => Assert.invariantFailed("NodeInfo.Exists, but arg was not a PathExpression.")
Review Comment:
It's annoying, but invariantFailed always wants the COVERAGE-ON/COVERAGE-OFF stuff around it.
I wish we could somehow build that in so it is implied for these assertions, but I don't know how.
Maybe there's a way to configure coverty so that it has a file that declares these sort of things so that one does not need to surround them?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,19 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
- val arg = args(0).asInstanceOf[PathExpression]
+ val arg = args(0) match {
+ case arg: PathExpression => arg
+ case _ => Assert.invariantFailed("NodeInfo.Exists, but arg was not a PathExpression.")
+ }
+ // test_lengthPatternBinaryPatternLimit in section 12 shows that the length can
+ // be evaluated when it is an array of hex binary values
+ if (arg.inherentType != NodeInfo.HexBinary && arg.isPathToOneWholeArray)
Review Comment:
This indicates a more significant issue.
The expresion "." when it corresponds to an element declaration that is an array, it does NOT denote the array, it denotes the one infoset node only.
So if the array element is name 'a', then "." is not equivalent to "../a" which references the whole array.
Furthermore, suppose you write ".[1]", and '.' corresonds to an element that is declared to be an array.
This was a subject of recent DFDL workgroup discussion, and the conclusion is that "." is never an array. So ".[1]" is always equivalent to just "." and refers to a single node only.
So I think the path expression "." (A PathExpression containing one step: Self) is going to need special treatment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1114416961
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2834,23 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
- val arg = args(0).asInstanceOf[PathExpression]
- val steps = arg.steps
+ val path = args(0) match {
+ case arg: PathExpression => arg
+ case _ => {
+ // $COVERAGE-OFF$
+ Assert.invariantFailed("NodeInfo.Exists, but arg was not a PathExpression.")
+ // $COVERAGE-ON$
+ }
+ }
+ // test_lengthPatternBinaryPatternLimit in section 12 shows that the length can
+ // be evaluated when it is an array of hex binary values
+ if (!path.isSelf && path.isPathToOneWholeArray)
Review Comment:
I moved the `path.isSelf` check to be inside `isPathToOneWholeArray` and that seems to work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on PR #966:
URL: https://github.com/apache/daffodil/pull/966#issuecomment-1437233606
> So I think the path expression "." (A PathExpression containing one step: Self) is going to need special treatment.
@mbeckerle Are you saying that this pull request needs an additional change? Can you help @mike-mcgann by being more specific where the additional change may be needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1114519090
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2834,23 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
- val arg = args(0).asInstanceOf[PathExpression]
- val steps = arg.steps
+ val path = args(0) match {
+ case arg: PathExpression => arg
+ case _ => {
+ // $COVERAGE-OFF$
+ Assert.invariantFailed("NodeInfo.Exists, but arg was not a PathExpression.")
+ // $COVERAGE-ON$
+ }
+ }
+ // test_lengthPatternBinaryPatternLimit in section 12 shows that the length can
+ // be evaluated when it is an array of hex binary values
+ if (!path.isSelf && path.isPathToOneWholeArray)
Review Comment:
This is quite clean now. I like it.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -387,7 +387,7 @@ object NodeInfo extends Enum {
}
/**
- * For things like fn:exists, fn:empty, dfdl:contenLength
+ * For things like fn:exists, fn:empty
Review Comment:
Not sure this comment should be changed. I think dfdl:contentLength and dfdl:valueLength should still be using NodeInfo.Exists as their required type for the first argument.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1113175654
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,19 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
- val arg = args(0).asInstanceOf[PathExpression]
+ val arg = args(0) match {
+ case arg: PathExpression => arg
+ case _ => Assert.invariantFailed("NodeInfo.Exists, but arg was not a PathExpression.")
Review Comment:
> It's annoying, but invariantFailed always wants the COVERAGE-ON/COVERAGE-OFF stuff around it. I wish we could somehow build that in so it is implied for these assertions, but I don't know how.
I bracketed the invariant with COVERAGE-ON/COVERAGE-OFF. I checked the docs but there doesn't seem to be a way to configure this at function invocation time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1114439480
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml:
##########
@@ -459,6 +459,37 @@
</xs:complexType>
</xs:element>
+ <xs:element name="valueLength14">
+ <xs:complexType>
+ <xs:sequence dfdl:separator="%WSP;">
+ <xs:element name="first" type="xs:string" dfdl:lengthKind="delimited"/>
+ <xs:element name="other" type="xs:string" dfdl:lengthKind="delimited" maxOccurs="unbounded"/>
+ <xs:element name="len" type="xs:unsignedInt" dfdl:inputValueCalc='{dfdl:valueLength(../ex:other, "bytes")}'/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="valueLength15" type="xs:unsignedInt" dfdl:inputValueCalc='{dfdl:valueLength("three", "bytes")}'/>
+
+ <xs:element name="valueLength16">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="e1">
Review Comment:
We might want to make e1 an array (e.g. `maxOccurs="unbounded"`) since this issue was all about getting the length of arrays, ande make sure a self expression in a complex really is referencing the right thing. Same with the contentLength6 schema.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #966: Improve errors with contentLength/valueLength (WIP)
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1109777236
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2131,7 +2131,7 @@ case class FunctionCallExpression(functionQNameString: String, expressions: List
functionQName,
args,
NodeInfo.Long,
- NodeInfo.Exists,
+ NodeInfo.Scalar,
Review Comment:
Right above this is a big comment about when "exists" makes sense. That needs to be removed/changed.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,15 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
+ if (!args(0).isInstanceOf[PathExpression]) {
+ SDE(s"expected a path expression but got type ${args(0).inherentType.globalQName}")
+ }
val arg = args(0).asInstanceOf[PathExpression]
Review Comment:
It feels like our type system should be able to handle this, but I don't think it can. The probably is it doesn't really differentiate between a literal string and a path to a string. In both cases, the arg expression is going to have an `inerhentType` of "String".
I wonder if our type systems needs the ability to differentiate that too? For example, we might have something like `NodeInfo.String` and `NodeInfo.ScalarString`. If we had this separate hiearchy of scalar-types and literal-types, paths would have an inherent type of the Scalar variant, and literals would have the inherent type of the non-Scalar variant. When doing conversions, a NodeString can convert to a String, but not the other way around. This way the SDE would get caugt in the `conversionOps` function.
If feels like maybe that's what Exists was accomplishing (since it's allow about requiring a node), but it does so without enough type information.
Note that this approach might allow more complex things like this:
```
dfdl:contentLength(if (foo) /path/to/one/thing else /path/to/another/thing, "bytes")
```
But wouldn't allow something like this:
```
dfdl:contentLength(if (foo) /path/to/one/thing else "bar", "bytes")
```
This would fail because the inherent type of the if-expression would convert to a String but contentLength would need a ScalarString, and String can't convert to ScalarString.
However, this idea feels pretty excessive for this problem, and even ignoring the fact that it doubles our types, I'm not sure it would actually work.
All that to say this check seems like a reasonable approach.
I'll add that since our type system doesn't help us with this problem, it probably means there are other functions where we allow literals where we shouldn't. For example, `fn:exists()` doesn't seem to check for this. There are likely others that we may need to look at, though that feels out of scope for this PR though. We might want to add an issue to add tests an fix those cases.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,15 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
+ if (!args(0).isInstanceOf[PathExpression]) {
+ SDE(s"expected a path expression but got type ${args(0).inherentType.globalQName}")
Review Comment:
Suggest capitalizing "e" in expected..
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -382,18 +380,28 @@ object NodeInfo extends Enum {
* All complex types are represented by this one type object.
*/
protected sealed trait ComplexKind extends AnyType.Kind
- case object Complex extends TypeNode('Complex, AnyType) with ComplexKind {
+ case object Complex extends TypeNode('Complex, Scalar) with ComplexKind {
type Kind = ComplexKind
}
/**
- * For things like fn:exists, fn:empty, dfdl:contenLength
+ * For things like fn:exists, fn:empty
*/
protected sealed trait ExistsKind extends AnyType.Kind
case object Exists extends TypeNode('Exists, AnyType) with AnyTypeKind {
type Kind = ExistsKind
}
+ /**
+ * For things like dfdl:contentLength, dfdl:valueLength
+ */
+ protected sealed trait ScalarKind extends AnyType.Kind
+ case object Scalar
+ extends TypeNode('Scalar, AnyType, Seq(AnySimpleType, Complex))
+ with AnyTypeKind {
+ type Kind = ScalarKind
+ }
Review Comment:
I believe this is correct. TypeNode basically just sets up a type hierarchy. AnyType is the parent of Scalar, and it's children are simple types and complex types.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -993,6 +1001,7 @@ object NodeInfo extends Enum {
lazy val allTypes =
allDFDLTypes ++ List(
Complex,
+ Scalar,
Array,
Review Comment:
Good point. Scalar's children are Complex (here in `allTypes`) and AnySimple (above in allAbstractTypes), so there doesn't seem to be an obvious place for. I don't think it currently matters because nothing actually uses `allAbstractTypes`, but it feels like Scalar is more like Exists/AnySimpleType than Complex and does want to be in the abstract lis.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on PR #966:
URL: https://github.com/apache/daffodil/pull/966#issuecomment-1439055196
> > The hexBinary test should then still work, but we should also add a test with a small simple complex type to show this working for that case also.
>
> It looks like test [valueLength11](https://github.com/apache/daffodil/blob/main/daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml#L418-L435) covers that condition.
That test doesn't use a self expression. vaLen also isn't an array using self. I think Mike was suggesting a similar test as the hexBinary one hat failed, but using a complex type instead of a simple hexbinary type?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1115788703
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -387,7 +387,7 @@ object NodeInfo extends Enum {
}
/**
- * For things like fn:exists, fn:empty, dfdl:contenLength
+ * For things like fn:exists, fn:empty
Review Comment:
Thanks for catching that one. Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Improve errors with contentLength/valueLength (WIP)
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1110081839
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,15 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
+ if (!args(0).isInstanceOf[PathExpression]) {
+ SDE(s"expected a path expression but got type ${args(0).inherentType.globalQName}")
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Add Scalar type
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1110214463
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -382,18 +380,28 @@ object NodeInfo extends Enum {
* All complex types are represented by this one type object.
*/
protected sealed trait ComplexKind extends AnyType.Kind
- case object Complex extends TypeNode('Complex, AnyType) with ComplexKind {
+ case object Complex extends TypeNode('Complex, Scalar) with ComplexKind {
type Kind = ComplexKind
}
/**
- * For things like fn:exists, fn:empty, dfdl:contenLength
+ * For things like fn:exists, fn:empty
*/
protected sealed trait ExistsKind extends AnyType.Kind
case object Exists extends TypeNode('Exists, AnyType) with AnyTypeKind {
type Kind = ExistsKind
}
+ /**
+ * For things like dfdl:contentLength, dfdl:valueLength
+ */
+ protected sealed trait ScalarKind extends AnyType.Kind
+ case object Scalar
+ extends TypeNode('Scalar, AnyType, Seq(AnySimpleType, Complex))
+ with AnyTypeKind {
+ type Kind = ScalarKind
+ }
Review Comment:
Changed back to Exists. I can go ahead and rename Exists to PathExpr in this PR if you think we should make that change now.
I found a isPathToOneWholeArray method in PathExpression which looks like it has the exact information needed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1115900729
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml:
##########
@@ -459,6 +459,37 @@
</xs:complexType>
</xs:element>
+ <xs:element name="valueLength14">
+ <xs:complexType>
+ <xs:sequence dfdl:separator="%WSP;">
+ <xs:element name="first" type="xs:string" dfdl:lengthKind="delimited"/>
+ <xs:element name="other" type="xs:string" dfdl:lengthKind="delimited" maxOccurs="unbounded"/>
+ <xs:element name="len" type="xs:unsignedInt" dfdl:inputValueCalc='{dfdl:valueLength(../ex:other, "bytes")}'/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="valueLength15" type="xs:unsignedInt" dfdl:inputValueCalc='{dfdl:valueLength("three", "bytes")}'/>
+
+ <xs:element name="valueLength16">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="e1">
Review Comment:
It has been moved up to the e1 element.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Improve errors with contentLength/valueLength (WIP)
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1110081445
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -993,6 +1001,7 @@ object NodeInfo extends Enum {
lazy val allTypes =
allDFDLTypes ++ List(
Complex,
+ Scalar,
Array,
Review Comment:
Moved to the abstract list
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2131,7 +2131,7 @@ case class FunctionCallExpression(functionQNameString: String, expressions: List
functionQName,
args,
NodeInfo.Long,
- NodeInfo.Exists,
+ NodeInfo.Scalar,
Review Comment:
Updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1110233357
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,19 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
- val arg = args(0).asInstanceOf[PathExpression]
+ val arg = args(0) match {
+ case arg: PathExpression => arg
+ case _ => Assert.invariantFailed("NodeInfo.Exists, but arg was not a PathExpression.")
+ }
+ // test_lengthPatternBinaryPatternLimit in section 12 shows that the length can
+ // be evaluated when it is an array of hex binary values
+ if (arg.inherentType != NodeInfo.HexBinary && arg.isPathToOneWholeArray)
Review Comment:
I'm not sure the hex binary exception is correct. In the mentioned test, I don't think the path evaluates to a whole array--the path is just `.` which in the context of where it's evaluated *I think* should just be a single element of that array (and this array just happens to be hexBinary, but it could be any type really). I think if `isPathToOneWholeArray` thinks the expression is evaluates to an array, then maybe there is a bug in that.
It seems like it things it's an array (which it kindof is), but in the context of this expression it really isn't...I'm not sure what the right fix is. Maybe it just needs exception for `Self`, since a Self expresion never can reference an array, but can only reference an element?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Add Scalar type
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1110218158
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,19 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
- val arg = args(0).asInstanceOf[PathExpression]
+ val arg = args(0) match {
+ case arg: PathExpression => arg
+ case _ => Assert.invariantFailed("NodeInfo.Exists, but arg was not a PathExpression.")
+ }
+ // test_lengthPatternBinaryPatternLimit in section 12 shows that the length can
+ // be evaluated when it is an array of hex binary values
+ if (arg.inherentType != NodeInfo.HexBinary && arg.isPathToOneWholeArray)
Review Comment:
See comment in the source above. Making this permissive for hexbinary values was needed for that test to pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on PR #966:
URL: https://github.com/apache/daffodil/pull/966#issuecomment-1437404440
> > So I think the path expression "." (A PathExpression containing one step: Self) is going to need special treatment.
>
> @mbeckerle Are you saying that this pull request needs an additional change? Can you help @mike-mcgann by being more specific where the additional change may be needed?
Yes, another change is needed. To be more specific. hexBinary is not what needs an exception in this code. We just have a test for hexBinary which calls `dfdl:valueLength(., "bytes")`.
The exception is not needed for hexBinary type, but for "." as the first argument to valueLength. For an array of *any* type, even a complex type, this expression `dfdl:valueLength(. , "bytes")` (or contentLength, and for any units: bits, bytes, or characters) needs to treat "." specially, because "." doesn't refer to an array, ever, regardless of how the corresponding element declaration is declared. "." always refers to exactly one infoset DIElement node.
I think the change is simple though. Add `def isSelf: Boolean` to PathExpression, and it is true if the path is just the Self step alone. Then use that to test for the exception, not the type.
The hexBinary test should then still work, but we should also add a test with a small simple complex type to show this working for that case also.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on PR #966:
URL: https://github.com/apache/daffodil/pull/966#issuecomment-1440151306
> That test doesn't use a self expression. vaLen also isn't an array using self. I think Mike was suggesting a similar test as the hexBinary one hat failed, but using a complex type instead of a simple hexbinary type?
I understand now. I've added those as contentLength_6 and valueLength_16
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Add Scalar type
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1110216194
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,15 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
+ if (!args(0).isInstanceOf[PathExpression]) {
Review Comment:
Added the match pattern.
Right now on the main branch there is a ClassCastException is the first argument to a function so there is something else wrong. I've filed ticket DAFFODIL-2795 for that issue and have commented out the relevant tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1115793502
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml:
##########
@@ -459,6 +459,37 @@
</xs:complexType>
</xs:element>
+ <xs:element name="valueLength14">
+ <xs:complexType>
+ <xs:sequence dfdl:separator="%WSP;">
+ <xs:element name="first" type="xs:string" dfdl:lengthKind="delimited"/>
+ <xs:element name="other" type="xs:string" dfdl:lengthKind="delimited" maxOccurs="unbounded"/>
+ <xs:element name="len" type="xs:unsignedInt" dfdl:inputValueCalc='{dfdl:valueLength(../ex:other, "bytes")}'/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="valueLength15" type="xs:unsignedInt" dfdl:inputValueCalc='{dfdl:valueLength("three", "bytes")}'/>
+
+ <xs:element name="valueLength16">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="e1">
Review Comment:
You have made v1 an array, bu the valueLength path references e1 which is still a scalar. Can the maxOccurs unbounded be moved to the e1 element so it's a complex array?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Improve errors with contentLength/valueLength (WIP)
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1110080907
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2113,7 +2113,7 @@ case class FunctionCallExpression(functionQNameString: String, expressions: List
functionQName,
args,
NodeInfo.Long,
- NodeInfo.Exists,
+ NodeInfo.Scalar,
NodeInfo.String,
DFDLContentLength(_),
)
Review Comment:
Updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a diff in pull request #966: Add Scalar type
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1110111507
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2113,7 +2113,7 @@ case class FunctionCallExpression(functionQNameString: String, expressions: List
functionQName,
args,
NodeInfo.Long,
- NodeInfo.Exists,
+ NodeInfo.Scalar,
NodeInfo.String,
DFDLContentLength(_),
)
Review Comment:
Pretty sure you don't need Scalar. Just use Exists.
Exists means "I don't care about the type, I just need it to be a PathExpression"
More about this below in other comments.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,15 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
+ if (!args(0).isInstanceOf[PathExpression]) {
+ SDE(s"expected a path expression but got type ${args(0).inherentType.globalQName}")
+ }
val arg = args(0).asInstanceOf[PathExpression]
Review Comment:
Steve is right, the type system cannot do this.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,15 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
+ if (!args(0).isInstanceOf[PathExpression]) {
Review Comment:
The code idiom you want is this:
```
args(0) match {
case arg: PathExpression => .. it is a path expression
case _ => Assert.invariantFailed("NodeInfo.Exists, but arg was not a PathExpression.")
}
```
If the function contentLength and valueLength use NodeInfo.Exists as their required type, then I believe the expression compiler should be insisting that the argument is a PathExpression, and if that doesn't happen, the expression compiler is broken elsewhere.
Now that's just disguising the cast differently, but it's cleaner looking.
More about this below in other comments.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -382,18 +380,28 @@ object NodeInfo extends Enum {
* All complex types are represented by this one type object.
*/
protected sealed trait ComplexKind extends AnyType.Kind
- case object Complex extends TypeNode('Complex, AnyType) with ComplexKind {
+ case object Complex extends TypeNode('Complex, Scalar) with ComplexKind {
type Kind = ComplexKind
}
/**
- * For things like fn:exists, fn:empty, dfdl:contenLength
+ * For things like fn:exists, fn:empty
*/
protected sealed trait ExistsKind extends AnyType.Kind
case object Exists extends TypeNode('Exists, AnyType) with AnyTypeKind {
type Kind = ExistsKind
}
+ /**
+ * For things like dfdl:contentLength, dfdl:valueLength
+ */
+ protected sealed trait ScalarKind extends AnyType.Kind
+ case object Scalar
+ extends TypeNode('Scalar, AnyType, Seq(AnySimpleType, Complex))
+ with AnyTypeKind {
+ type Kind = ScalarKind
+ }
Review Comment:
Scalar is the opposite of Array, so I think this should be like Array, no children, no parent.
But really we only need Exists.
I'm pretty sure we could get rid of the NodeInfo.Array easily, as it is barely used anywhere. I think it was my mistake to put Array in this taxonomy originally. I don't think it is actually used.
The things I've seen that care if things are arrays or not aren't using NodeInfo objects or scala types like Array.Kind, they're calling PathExpression.steps.last.isArray, and I think that's what you want also.
Actually I think you should add
```
def isArray: Boolean = steps.last.isArray
def isScalar: Boolean = !isArray
```
to PathExpression.
Exists means "I don't care about the type" what it does care about is that the expression is a path expression i.e., where one can evaluate "does this exist".
You want a minor variation on that where you can also ask if it's array or not.
Dimensionality in DFDL/XSD is orthogonal to type. I've always known that, but somehow decided a loooong time ago (like 2011) to put Array into the type hierarchy. That looks like a mistake now.
Perhaps NodeInfo.Exists should be renamed to NodeInfo.PathExpr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a diff in pull request #966: Improve errors with contentLength/valueLength (WIP)
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1107788482
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,15 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
+ if (!args(0).isInstanceOf[PathExpression]) {
+ SDE(s"expected a path expression but got type ${args(0).inherentType.globalQName}")
+ }
val arg = args(0).asInstanceOf[PathExpression]
Review Comment:
> I added an additional test to ensure there is an error when a string literal is used as the first argument. That causes a ClassCastException. I wasn't sure how to restrict that from happening using the type system and I just hacked in a type check to instead throw an SDE. I need some suggestions on how to implement this fix properly.
Is this the place you mentioned where you want a better suggestion how to avoid the ClassCastException from the asInstanceOf[PathExpression] on line 2835? Your type check looks fine to me but any thoughts, Steve? The only suggestion I have is to call schemaDefinitionUnless instead which collapses the if and SDE into a single function call. This file already has 27 SDE calls and 9 schemaDefinitionUnless calls, so your type check will be in good company.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -382,18 +380,28 @@ object NodeInfo extends Enum {
* All complex types are represented by this one type object.
*/
protected sealed trait ComplexKind extends AnyType.Kind
- case object Complex extends TypeNode('Complex, AnyType) with ComplexKind {
+ case object Complex extends TypeNode('Complex, Scalar) with ComplexKind {
type Kind = ComplexKind
}
/**
- * For things like fn:exists, fn:empty, dfdl:contenLength
+ * For things like fn:exists, fn:empty
*/
protected sealed trait ExistsKind extends AnyType.Kind
case object Exists extends TypeNode('Exists, AnyType) with AnyTypeKind {
type Kind = ExistsKind
}
+ /**
+ * For things like dfdl:contentLength, dfdl:valueLength
+ */
+ protected sealed trait ScalarKind extends AnyType.Kind
+ case object Scalar
+ extends TypeNode('Scalar, AnyType, Seq(AnySimpleType, Complex))
+ with AnyTypeKind {
+ type Kind = ScalarKind
+ }
Review Comment:
I'm not familiar with the TypeNode constructor, so Steve please take a look too.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2113,7 +2113,7 @@ case class FunctionCallExpression(functionQNameString: String, expressions: List
functionQName,
args,
NodeInfo.Long,
- NodeInfo.Exists,
+ NodeInfo.Scalar,
NodeInfo.String,
DFDLContentLength(_),
)
Review Comment:
Please find the comment on the next line (not shown in the diff) and update it so it no longer refers to `Exists` but refers to `Scalar` instead.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -993,6 +1001,7 @@ object NodeInfo extends Enum {
lazy val allTypes =
allDFDLTypes ++ List(
Complex,
+ Scalar,
Array,
Review Comment:
I notice Exists is listed in allAbstractTypes (above at line 969) while Scalar is listed in allTypes here. I'm sure Scalar is more analogous to Complex than Exists, but I thought I should mention it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on PR #966:
URL: https://github.com/apache/daffodil/pull/966#issuecomment-1438630902
> I think the change is simple though. Add `def isSelf: Boolean` to PathExpression, and it is true if the path is just the Self step alone. Then use that to test for the exception, not the type.
Updated.
> The hexBinary test should then still work, but we should also add a test with a small simple complex type to show this working for that case also.
It looks like test [valueLength11](https://github.com/apache/daffodil/blob/main/daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml#L418-L435) covers that condition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1113534864
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2834,23 @@ sealed abstract class LengthExprBase(
) {
protected final def leafReferencedElements = {
- val arg = args(0).asInstanceOf[PathExpression]
- val steps = arg.steps
+ val path = args(0) match {
+ case arg: PathExpression => arg
+ case _ => {
+ // $COVERAGE-OFF$
+ Assert.invariantFailed("NodeInfo.Exists, but arg was not a PathExpression.")
+ // $COVERAGE-ON$
+ }
+ }
+ // test_lengthPatternBinaryPatternLimit in section 12 shows that the length can
+ // be evaluated when it is an array of hex binary values
+ if (!path.isSelf && path.isPathToOneWholeArray)
Review Comment:
Does the `path.isSelf` condition want to be checked inside `isPathToOneWholeArray`. A self path can never be a path to a whole array, so I imagine we would need this same check anywhere `isPathToOneWholeArray`, or we could just include it in he logic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1115900729
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml:
##########
@@ -459,6 +459,37 @@
</xs:complexType>
</xs:element>
+ <xs:element name="valueLength14">
+ <xs:complexType>
+ <xs:sequence dfdl:separator="%WSP;">
+ <xs:element name="first" type="xs:string" dfdl:lengthKind="delimited"/>
+ <xs:element name="other" type="xs:string" dfdl:lengthKind="delimited" maxOccurs="unbounded"/>
+ <xs:element name="len" type="xs:unsignedInt" dfdl:inputValueCalc='{dfdl:valueLength(../ex:other, "bytes")}'/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="valueLength15" type="xs:unsignedInt" dfdl:inputValueCalc='{dfdl:valueLength("three", "bytes")}'/>
+
+ <xs:element name="valueLength16">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="e1">
Review Comment:
Updated and moved up to the e1 element.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence merged pull request #966: Better error message when contentLength/valueLength argument is an array
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence merged PR #966:
URL: https://github.com/apache/daffodil/pull/966
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org