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/24 15:56:11 UTC
[GitHub] [daffodil] mike-mcgann opened a new pull request, #972: Update occursIndex to skip absent elements
mike-mcgann opened a new pull request, #972:
URL: https://github.com/apache/daffodil/pull/972
The occursIndex function should return the current index of the element in the infoset. Daffodil was using the current array position as this value. When the emptyElementParsePolicy is set to treatAsAbsent, empty elements are not included in the final infoset and therefore the current array position may not match the index in the final infoset.
This change introduces a new variable in the parser and unparser states for tracking the current occusIndex value which is not incremented when an absent element is seen.
[DAFFODIL-2791](https://issues.apache.org/jira/browse/DAFFODIL-2791)
--
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 #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1118787393
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/PState.scala:
##########
@@ -108,12 +113,16 @@ object MPState {
class MPState private () {
val arrayIndexStack = MStackOfLong()
Review Comment:
It is kind of iterating over one array and creating a new array which may or may not be the same length. How about naming these two to be `arraySourcePos` and `arrayTargetPos`?
--
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 #972: Update occursIndex to skip absent elements
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1117324737
##########
daffodil-test/src/test/scala/org/apache/daffodil/usertests/TestSepTests.scala:
##########
@@ -70,6 +70,8 @@ class TestSepTests {
@Test def test_sep_ssp_never_5(): Unit = { runner.runOneTest("test_sep_ssp_never_5") }
// DAFFODIL-2791
- @Test def test_treatAsMissing_occursIndex(): Unit = { runner.runOneTest("test_treatAsMissing_occursIndex") }
Review Comment:
This line doesn't seem too long to me. Fits fine on my screen for github code review and my browser is zoomed to 133%.
Should we allow longer lines?
--
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 #972: Update occursIndex to skip absent elements
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1120459350
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/PState.scala:
##########
@@ -107,12 +112,16 @@ object MPState {
class MPState private () {
- val arrayIndexStack = MStackOfLong()
+ val arrayIterationIndexStack = MStackOfLong()
+ val occursIndexStack = MStackOfLong()
+
+ def moveOverOneArrayIterationIndexOnly() =
+ arrayIterationIndexStack.push(arrayIterationIndexStack.pop + 1)
- def moveOverOneArrayIndexOnly() = arrayIndexStack.push(arrayIndexStack.pop + 1)
- def moveBackOneArrayIndexOnly() = arrayIndexStack.push(arrayIndexStack.pop - 1)
+ def arrayPos = arrayIterationIndexStack.top
Review Comment:
Can this be renamed `arrayIterationPos`?
--
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 #972: Update occursIndex to skip absent elements
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1117444383
##########
daffodil-test/src/test/scala/org/apache/daffodil/usertests/TestSepTests.scala:
##########
@@ -70,6 +70,8 @@ class TestSepTests {
@Test def test_sep_ssp_never_5(): Unit = { runner.runOneTest("test_sep_ssp_never_5") }
// DAFFODIL-2791
- @Test def test_treatAsMissing_occursIndex(): Unit = { runner.runOneTest("test_treatAsMissing_occursIndex") }
Review Comment:
I'm using a laptop screen since I'm away from both home and office now, so I see only this much of the line and I would have to use the horizontal scrollbar to see the rest of the line (my browser is zoomed to 100%):
```scala
@Test def test_treatAsMissing_occursIndex(): Unit = { runner.runOneTest("test_treatAsMissing_occursI
```
BTW, I checked and found out if I increase maxColumn to 100, sbt scalafmtCheckAll fails and scalafmtAll reformats a bunch of files to re-join some files that had been split before. Any time we change the allowed length, we'll reformat some files.
--
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 merged pull request #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann merged PR #972:
URL: https://github.com/apache/daffodil/pull/972
--
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 #972: Update occursIndex to skip absent elements
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on PR #972:
URL: https://github.com/apache/daffodil/pull/972#issuecomment-1448714647
Looking at the CI actions, installing the libxml dependency and setting up Java takes 8 seconds combined. And that's all there is as far as dependency set up. SBT and other build dependencies are already included in the ubuntu image we use. I can't tell how long it takes GitHub actions to actually download and load the ubuntu image, but it's a default hosted runner from GitHub so it's probably heavily cached.
I'm not against a dev container, but saving a few seconds in CI probably isn't worth the maintenance burden of having to periodically update the image when dependencies/java version updates--it's much easier to just bump a number in a text file and let GitHub handle the rest.
--
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 #972: Update occursIndex to skip absent elements
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1120646738
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/debugger/InteractiveDebugger.scala:
##########
@@ -1837,7 +1837,7 @@ class InteractiveDebugger(
val longDesc = desc
def getSomeValue(state: StateForDebugger): Option[Long] = {
- if (state.arrayPos != -1) Some(state.arrayPos) else None
+ if (state.arrayIterationPos != -1) Some(state.arrayIterationPos) else None
Review Comment:
Should this return `occursIndexPos`? The debugger command is called `occursIndex` so that might be what users expect?
--
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 #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on PR #972:
URL: https://github.com/apache/daffodil/pull/972#issuecomment-1448692000
> I wouldn't want to have to maintain multiple docker images for CI tests. We already have 7 different OS/JDK combinations we have.
We only need one for linux and latest and greatest JDK. It could be used for that build and for the lint check. The other combos could be done as it is now. It might be faster for CI to download the image than to install the dependencies and there is a chance that the CI could cache the image.
--
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 #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1120711266
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/debugger/InteractiveDebugger.scala:
##########
@@ -1837,7 +1837,7 @@ class InteractiveDebugger(
val longDesc = desc
def getSomeValue(state: StateForDebugger): Option[Long] = {
- if (state.arrayPos != -1) Some(state.arrayPos) else None
+ if (state.arrayIterationPos != -1) Some(state.arrayIterationPos) else None
Review Comment:
This was an auto-refactor here. I went ahead and updated it to use occursPos.
--
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 #972: Update occursIndex to skip absent elements
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on PR #972:
URL: https://github.com/apache/daffodil/pull/972#issuecomment-1448550336
> I wonder if it would be worth moving checks that aren't specific to OS and JDK versions into their own "lint" job inside main.yml?
Agreed. It would also make the OS/JDK jobs finish 1-2 minutes quicker. Not a huge difference, but the quicker tests finish the better. The new job could also be configured to always run all checks even if one fails.
> At that time it would also be nice to see if we can have the tests run in a docker container with all the dependencies and configurations installed and ready-to-go.
I wouldn't want to have to maintain multiple docker images for CI tests. We already have 7 different OS/JDK combinations we have. Though I agree that a dev container could be useful. Though, most of the dependency work is handled by sbt which is straightforward to install. I think the only deps it doesn't handle is gcc and MiniXML dep, which I think most distros already provide.
Whatever is decided can all be done as a separate PR though.
--
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 #972: Update occursIndex to skip absent elements
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1117444383
##########
daffodil-test/src/test/scala/org/apache/daffodil/usertests/TestSepTests.scala:
##########
@@ -70,6 +70,8 @@ class TestSepTests {
@Test def test_sep_ssp_never_5(): Unit = { runner.runOneTest("test_sep_ssp_never_5") }
// DAFFODIL-2791
- @Test def test_treatAsMissing_occursIndex(): Unit = { runner.runOneTest("test_treatAsMissing_occursIndex") }
Review Comment:
I'm using a laptop screen since I'm away from both home and office now, so I see only this much of the line and I would have to use the horizontal scrollbar to see the rest of the line (my browser is zoomed to 100%):
```scala
@Test def test_treatAsMissing_occursIndex(): Unit = { runner.runOneTest("test_treatAsMissing_occursI
```
BTW, I checked and found out if I increase maxColumn to 100, sbt scalafmtCheckAll fails and scalafmtAll reformats a bunch of files to re-join some lines that had been split before. Any time we change the allowed length, we'll reformat some files.
--
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 #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1120236754
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/ProcessorStateBases.scala:
##########
@@ -507,6 +507,7 @@ abstract class ParseOrUnparseState protected (
def groupPos: Long
def arrayPos: Long
+ def occursIndex: Long
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] mike-mcgann commented on a diff in pull request #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1120611130
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/PState.scala:
##########
@@ -107,12 +112,16 @@ object MPState {
class MPState private () {
- val arrayIndexStack = MStackOfLong()
+ val arrayIterationIndexStack = MStackOfLong()
+ val occursIndexStack = MStackOfLong()
+
+ def moveOverOneArrayIterationIndexOnly() =
+ arrayIterationIndexStack.push(arrayIterationIndexStack.pop + 1)
- def moveOverOneArrayIndexOnly() = arrayIndexStack.push(arrayIndexStack.pop + 1)
- def moveBackOneArrayIndexOnly() = arrayIndexStack.push(arrayIndexStack.pop - 1)
+ def arrayPos = arrayIterationIndexStack.top
Review Comment:
Thanks for finding that one. I should have done a grep for arrayPos.
--
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 #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on PR #972:
URL: https://github.com/apache/daffodil/pull/972#issuecomment-1448525699
> I wonder if it would be worth moving checks that aren't specific to OS and JDK versions into their own "lint" job inside main.yml? It must be discouraging to make a change to a PR and see 3 out of 6 matrix jobs fail across the board.
Yeah, I think that is a better long-term solution. At that time it would also be nice to see if we can have the tests run in a docker container with all the dependencies and configurations installed and ready-to-go. A bonus to that is having a image you can use locally that is the same one used in the CI build.
--
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 #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1120233088
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/PState.scala:
##########
@@ -108,12 +113,16 @@ object MPState {
class MPState private () {
val arrayIndexStack = MStackOfLong()
Review Comment:
I went ahead and put in the first suggestion of `arrayIterationIndexStack` and `arrayIterationPos`. We can easily refactor the name if we come up with a better one.
--
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 #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1118857657
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceParserBases.scala:
##########
@@ -194,6 +194,12 @@ abstract class SequenceParserBase(
pstate.mpstate.moveOverOneArrayIndexOnly()
}
+ // If the emptyElementParsePolicy is set to treatAsAbsent, don't
+ // increment the occursIndex if the element is absent
+ if (resultOfTry != AbsentRep) {
Review Comment:
I was assuming that if treatAsAbsent was not set that it would be EmptyRep instead which I think is the case? I did try to see if I could check the emptyElementParsePolicy at that point but I wasn't able to navigate the object tree to find 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 a diff in pull request #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1120237951
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceParserBases.scala:
##########
@@ -194,6 +194,12 @@ abstract class SequenceParserBase(
pstate.mpstate.moveOverOneArrayIndexOnly()
}
+ // If the emptyElementParsePolicy is set to treatAsAbsent, don't
+ // increment the occursIndex if the element is absent
+ if (resultOfTry != AbsentRep ) {
+ pstate.mpstate.incrementOccursIndex()
+ }
Review Comment:
There was no actual reason for that. I moved the absent check into the `ais eq Done` block
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/unparsers/UState.scala:
##########
@@ -459,7 +464,8 @@ final class UStateForSuspension(
override def groupPos = 0 // was die, but this is called when copying state during debugging
override def currentInfosetNodeMaybe = Maybe(currentInfosetNode)
- override def arrayPos = occursIndex
+ override def arrayPos = arrayIndex
+ override def occursIndex = occurs
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] mike-mcgann commented on a diff in pull request #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1120234511
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/PState.scala:
##########
@@ -108,12 +113,16 @@ object MPState {
class MPState private () {
val arrayIndexStack = MStackOfLong()
+ val occursIndexStack = MStackOfLong()
def moveOverOneArrayIndexOnly() = arrayIndexStack.push(arrayIndexStack.pop + 1)
def moveBackOneArrayIndexOnly() = arrayIndexStack.push(arrayIndexStack.pop - 1)
def arrayPos = arrayIndexStack.top
+ def incrementOccursIndex() = occursIndexStack.push(occursIndexStack.pop + 1)
Review Comment:
Updated to `moverOverOneOccursIndexOnly` and removed `moveBackOneArrayIndex`
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/unparsers/UState.scala:
##########
@@ -413,7 +415,8 @@ final class UStateForSuspension(
override var dataOutputStream: DirectOrBufferedDataOutputStream,
vbox: VariableBox,
override val currentInfosetNode: DINode,
- occursIndex: Long,
+ arrayIndex: Long,
+ occurs: Long,
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 #972: Update occursIndex to skip absent elements
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1117288944
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/DPathRuntime.scala:
##########
@@ -64,6 +64,7 @@ class CompiledDPath(val ops: RecipeOp*) extends Serializable {
}
dstate.setArrayPos(state.arrayPos)
Review Comment:
Can we remove `arrayPos` from the DState? I don't think anything in expressions needs that information anymore. It only ever needs the occursPos.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/PState.scala:
##########
@@ -108,12 +113,16 @@ object MPState {
class MPState private () {
val arrayIndexStack = MStackOfLong()
+ val occursIndexStack = MStackOfLong()
def moveOverOneArrayIndexOnly() = arrayIndexStack.push(arrayIndexStack.pop + 1)
def moveBackOneArrayIndexOnly() = arrayIndexStack.push(arrayIndexStack.pop - 1)
def arrayPos = arrayIndexStack.top
+ def incrementOccursIndex() = occursIndexStack.push(occursIndexStack.pop + 1)
Review Comment:
For consistency, I'd suggest naming this `moveOverOneOccursIndexOnly`.
Why don't we need a `moveBackOnOccursIndexOnly`? I would imagine in cases where we decrement aray index we might also need to decrement occurs index?
Actually, it looks like nothing calls moveBackOneArrayIndex, so maybe we should just remove that.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/PState.scala:
##########
@@ -108,12 +113,16 @@ object MPState {
class MPState private () {
val arrayIndexStack = MStackOfLong()
Review Comment:
Is it worth renaming this to something like `arrayIterationIndexStack` to make it more clear that it is the array iteration count and not the actual index of the current array element, since the two aren't necessarily the same.
I also wonder if we want to do the same with `arrayPos` and rename it to `arrayIterationPos`? I think `arrayPos` is even more confusing as a name, since it really does make me think it's the position in the array, which is isn't always the case.
I think this PR would be a good time to look at places where `arrayPos` is used and make sure it's used correctly, since it's possible `dfdl:occursIndex` isn't the only case.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/ProcessorStateBases.scala:
##########
@@ -507,6 +507,7 @@ abstract class ParseOrUnparseState protected (
def groupPos: Long
def arrayPos: Long
+ def occursIndex: Long
Review Comment:
Should this be occursPos to be consistent with other naming conventions?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/unparsers/UState.scala:
##########
@@ -459,7 +464,8 @@ final class UStateForSuspension(
override def groupPos = 0 // was die, but this is called when copying state during debugging
override def currentInfosetNodeMaybe = Maybe(currentInfosetNode)
- override def arrayPos = occursIndex
+ override def arrayPos = arrayIndex
+ override def occursIndex = occurs
Review Comment:
Suggest occursPos for consisence.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/unparsers/UState.scala:
##########
@@ -413,7 +415,8 @@ final class UStateForSuspension(
override var dataOutputStream: DirectOrBufferedDataOutputStream,
vbox: VariableBox,
override val currentInfosetNode: DINode,
- occursIndex: Long,
+ arrayIndex: Long,
+ occurs: Long,
Review Comment:
Suggest `occursIndex` for consistency.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceParserBases.scala:
##########
@@ -194,6 +194,12 @@ abstract class SequenceParserBase(
pstate.mpstate.moveOverOneArrayIndexOnly()
}
+ // If the emptyElementParsePolicy is set to treatAsAbsent, don't
+ // increment the occursIndex if the element is absent
+ if (resultOfTry != AbsentRep ) {
+ pstate.mpstate.incrementOccursIndex()
+ }
Review Comment:
This is the only difference between arrayIndex and occursIndex, right? Why do we increment occursIndex when ais eq Done? We exclude that for arrayIndex for some reason?
--
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 #972: Update occursIndex to skip absent elements
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1117333727
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceParserBases.scala:
##########
@@ -194,6 +194,12 @@ abstract class SequenceParserBase(
pstate.mpstate.moveOverOneArrayIndexOnly()
}
+ // If the emptyElementParsePolicy is set to treatAsAbsent, don't
+ // increment the occursIndex if the element is absent
+ if (resultOfTry != AbsentRep) {
Review Comment:
Does resultOfTry only come back AbsentRep if treatAsAbsent is set?
I'm not sure that's the case. If so then ok.
I expected this to test resultOfTry as you have, but also to inspect the property (which has to be captured onto the runtime data structures) to see if it is treatAsAbsent.
--
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 #972: Update occursIndex to skip absent elements
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1120233683
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/DPathRuntime.scala:
##########
@@ -64,6 +64,7 @@ class CompiledDPath(val ops: RecipeOp*) extends Serializable {
}
dstate.setArrayPos(state.arrayPos)
Review Comment:
I removed it and it caused no problems
--
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 #972: Update occursIndex to skip absent elements
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on PR #972:
URL: https://github.com/apache/daffodil/pull/972#issuecomment-1448439341
I wonder if it would be worth moving checks that aren't specific to OS and JDK versions into their own "lint" job inside main.yml? It must be discouraging to make a change to a PR and see 3 out of 6 matrix jobs fail across the board. The Rat, OSGi, scalafmt, and sonarcloud checks seem like they could run in their own job just fine. If we run these checks asynchronously in their own job, I suspect we want to allow the matrix jobs to complete even if the lint job fails since we still want to know whether the unit and integration tests pass (the more information the better), yes?
--
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 #972: Update occursIndex to skip absent elements
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1119122559
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceParserBases.scala:
##########
@@ -194,6 +194,12 @@ abstract class SequenceParserBase(
pstate.mpstate.moveOverOneArrayIndexOnly()
}
+ // If the emptyElementParsePolicy is set to treatAsAbsent, don't
+ // increment the occursIndex if the element is absent
+ if (resultOfTry != AbsentRep) {
Review Comment:
Ok. I suspect you are right, and if tests aren't breaking on account of this, then we are almost certainly ok.
--
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 #972: Update occursIndex to skip absent elements
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #972:
URL: https://github.com/apache/daffodil/pull/972#discussion_r1119128788
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/PState.scala:
##########
@@ -108,12 +113,16 @@ object MPState {
class MPState private () {
val arrayIndexStack = MStackOfLong()
Review Comment:
I'm not sure source and target are obvious to me, especially since it could be confusing depending on parse vs unparse. For example, when parsing the source is data but when unparsing the source is the infoset. But along the same lines, maybe using infoset/data would make the distinction more obivous? So maybe something like arrayDataPos and arrayInfosetPos?
@mbeckerle, I skimmed the DFDL spec but couldn't really find any obvious terms to steal from DFDL. Maybe DFDL already has concepts for this kind of thing?
--
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