You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "mbeckerle (via GitHub)" <gi...@apache.org> on 2024/05/29 23:45:50 UTC
[PR] Add requireLengthInWholeWords feature to byteSwap layer [daffodil]
mbeckerle opened a new pull request, #1248:
URL: https://github.com/apache/daffodil/pull/1248
Eliminates the need for a separate byteSwap layer just to get an error if the data is not a mulitple of the word size.
DAFFODIL-2905
--
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
Re: [PR] Add requireLengthInWholeWords feature to byteSwap layer [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1248:
URL: https://github.com/apache/daffodil/pull/1248#discussion_r1620795360
##########
daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1/ByteSwapLayer.scala:
##########
@@ -137,14 +180,15 @@ class ByteSwapInputStream(wordsize: Int, jis: InputStream) extends InputStream {
* order 4 3 2 1 8 7 6 5 10 9. If wordsize were 2 then the bytes are written
* to the wrapped output stream in the order 2 1 4 3 6 5 8 7 10 9.
*/
-class ByteSwapOutputStream(wordsize: Int, jos: OutputStream) extends OutputStream {
+class ByteSwapOutputStream(layer: Layer, wordsize: Int, jos: OutputStream)
+ extends OutputStream {
- private val stack: Deque[Byte] = new ArrayDeque[Byte](wordsize)
+ private val stack: Deque[Byte] = new util.ArrayDeque[Byte](wordsize)
private var closed = false
override def close(): Unit = {
if (!closed) {
- while (!stack.isEmpty()) {
+ while (!stack.isEmpty) {
Review Comment:
Added check and test that expects the error during unparse.
--
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
Re: [PR] Add requireLengthInWholeWords feature to byteSwap layer [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1248:
URL: https://github.com/apache/daffodil/pull/1248#discussion_r1620823622
##########
daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1/ByteSwapLayer.scala:
##########
@@ -29,18 +30,47 @@ final class TwoByteSwapLayer extends ByteSwap("twobyteswap", 2)
final class FourByteSwapLayer extends ByteSwap("fourbyteswap", 4)
-abstract class ByteSwap(name: String, count: Int)
+/**
+ * This class represents a byte swap layer that can be used to swap the byte order of data.
+ *
+ * DFDL Variable `requireLengthInWholeWords` can be used to request that the layer enforce
+ * the length being a multiple of the word size.
+ *
+ * @constructor Creates a new ByteSwap instance with the specified name and word size.
+ * @param name The name of the byte swap layer.
+ * @param wordsize The word size in bytes.
+ */
+abstract class ByteSwap(name: String, wordsize: Int)
extends Layer(name, "urn:org.apache.daffodil.layers.byteSwap") {
+ private var wholeWords: Boolean = false
+
+ /**
+ * Initialize from DFDL variables that are parameters.
+ * @param requireLengthInWholeWords a string that is the value of the DFDL variable of the same name in this layer's
+ * namespace. Must be "yes" or "no" or it is a SDE.
+ */
+ def setLayerVariableParameters(requireLengthInWholeWords: String): Unit = {
+ requireLengthInWholeWords match {
+ case "yes" => this.wholeWords = true
+ case "no" => this.wholeWords = false // this is the default
Review Comment:
I made it a string with yes/no for this extensibility reason. I think that's sufficient.
--
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
Re: [PR] Add requireLengthInWholeWords feature to byteSwap layer [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1248:
URL: https://github.com/apache/daffodil/pull/1248#discussion_r1619788199
##########
daffodil-runtime1-layers/src/main/resources/org/apache/daffodil/layers/xsd/byteSwapLayer.dfdl.xsd:
##########
@@ -26,12 +26,10 @@
<annotation>
<appinfo source="http://www.ogf.org/dfdl/">
- <!--
- The twobyteswap and fourbyteswap layers have no parameters nor
- return variables
- -->
+ <!-- set/bind to yes if you want a parse error when the length is not a multiple of the word size -->
+ <dfdl:defineVariable name="requireLengthInWholeWords" type="xs:string" defaultValue="no"/>
Review Comment:
This is a good example of why it's useful to have a convention where each layer has a .dfdl.xsd file and schema import these files, even if they are empty. Not only does it provide a place to add variables if the layer changes, but it also allows schema compatibility with new versions of Daffodil. Say a schema was written with the old layer and it imports the empty file. If it later upgrade to a new version of Daffodil then it will import the same file but with the new defineVariable and default value, so no changes to the schema a required to use the updated layer.
##########
daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1/ByteSwapLayer.scala:
##########
@@ -137,14 +180,15 @@ class ByteSwapInputStream(wordsize: Int, jis: InputStream) extends InputStream {
* order 4 3 2 1 8 7 6 5 10 9. If wordsize were 2 then the bytes are written
* to the wrapped output stream in the order 2 1 4 3 6 5 8 7 10 9.
*/
-class ByteSwapOutputStream(wordsize: Int, jos: OutputStream) extends OutputStream {
+class ByteSwapOutputStream(layer: Layer, wordsize: Int, jos: OutputStream)
+ extends OutputStream {
- private val stack: Deque[Byte] = new ArrayDeque[Byte](wordsize)
+ private val stack: Deque[Byte] = new util.ArrayDeque[Byte](wordsize)
private var closed = false
override def close(): Unit = {
if (!closed) {
- while (!stack.isEmpty()) {
+ while (!stack.isEmpty) {
Review Comment:
If `requireWholeWords` is true when closing, should this error if the stack is not empty? I think a non-empty stack here implies the underlying output stream didn't write a multiple of wordsize bytes and so we are missing a whole word?
##########
daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1/ByteSwapLayer.scala:
##########
@@ -19,6 +19,7 @@ package org.apache.daffodil.layers.runtime1
import java.io.InputStream
import java.io.OutputStream
+import java.util
Review Comment:
Can we remove this import? This is only used once for `new util.ArrayDeque` and we already import that below.
##########
daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1/ByteSwapLayer.scala:
##########
@@ -29,18 +30,47 @@ final class TwoByteSwapLayer extends ByteSwap("twobyteswap", 2)
final class FourByteSwapLayer extends ByteSwap("fourbyteswap", 4)
-abstract class ByteSwap(name: String, count: Int)
+/**
+ * This class represents a byte swap layer that can be used to swap the byte order of data.
+ *
+ * DFDL Variable `requireLengthInWholeWords` can be used to request that the layer enforce
+ * the length being a multiple of the word size.
+ *
+ * @constructor Creates a new ByteSwap instance with the specified name and word size.
+ * @param name The name of the byte swap layer.
+ * @param wordsize The word size in bytes.
+ */
+abstract class ByteSwap(name: String, wordsize: Int)
extends Layer(name, "urn:org.apache.daffodil.layers.byteSwap") {
+ private var wholeWords: Boolean = false
+
+ /**
+ * Initialize from DFDL variables that are parameters.
+ * @param requireLengthInWholeWords a string that is the value of the DFDL variable of the same name in this layer's
+ * namespace. Must be "yes" or "no" or it is a SDE.
+ */
+ def setLayerVariableParameters(requireLengthInWholeWords: String): Unit = {
+ requireLengthInWholeWords match {
+ case "yes" => this.wholeWords = true
+ case "no" => this.wholeWords = false // this is the default
Review Comment:
Would it be useful to have something other than yes/no? For example, I'm thinking maybe an additional value could allow non-whole words on parse but pad to a whole word on unparse. I'm not sure we need that behavior, but making the variable name something like `nonWholeWordPolicy` (not sure about that name) could have accept/error instead of yes/no, but could let you extend it in the future to implement something like "acceptAndPad". Though I guess `requirLengthInWholeWords="noButPad"` could still work, so I guess as long as its a string its fine?
--
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
Re: [PR] Add requireLengthInWholeWords feature to byteSwap layer [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle merged PR #1248:
URL: https://github.com/apache/daffodil/pull/1248
--
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
Re: [PR] Add requireLengthInWholeWords feature to byteSwap layer [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1248:
URL: https://github.com/apache/daffodil/pull/1248#discussion_r1620869640
##########
daffodil-test/src/test/resources/org/apache/daffodil/layers/xsd/testTwoByteSwap.dfdl.xsd:
##########
@@ -0,0 +1,90 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<schema xmlns="http://www.w3.org/2001/XMLSchema"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema"
+ xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:dfdlx="http://www.ogf.org/dfdl/dfdl-1.0/extensions"
+ xmlns:fn="http://www.w3.org/2005/xpath-functions"
+ xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+ xmlns:twobyteSwap="urn:org.apache.daffodil.layers.byteSwap"
+ xmlns:ex="http://example.com"
+ targetNamespace="http://example.com">
+
+ <include schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+
+ <xs:import namespace="urn:org.apache.daffodil.layers.byteSwap"
+ schemaLocation="/org/apache/daffodil/layers/xsd/byteSwapLayer.dfdl.xsd"/>
+
+ <annotation>
+ <appinfo source="http://www.ogf.org/dfdl/">
+ <dfdl:format ref="ex:GeneralFormat"
+ representation="binary"
+ lengthUnits="bits"
+ />
+ </appinfo>
+ </annotation>
+
+ <group name="setVars">
+ <sequence>
+ <annotation><appinfo source="http://www.ogf.org/dfdl/">
+ <dfdl:setVariable ref="twobyteSwap:requireLengthInWholeWords" value="yes"/>
+ </appinfo></annotation>
+ </sequence>
+ </group>
+
+ <element name="TwoByteSwapTest">
+ <complexType>
+ <sequence>
+ <group ref="ex:setVars"/>
+ <element name="len5" dfdl:lengthKind="explicit" dfdl:length="5"
+ dfdl:lengthUnits="bytes"><!-- note odd fixed length -->
+ <complexType>
+ <sequence>
+ <sequence dfdlx:layer="twobyteSwap:twobyteswap">
+ <element name="Block">
+ <complexType>
+ <sequence>
+ <!-- this will not pull data from the underlying layer 2 bytes at a time.
+ Rather, the I/O layer tries to fill this and hits the end of data and detects odd length so
+ causes a PE which ends the array with zero elements in it.
+ That will not cause the layer to fail, just to backtrack to the start of the layer data. -->
+ <element name="Data" type="unsignedShort"
+ minOccurs="2"
+ maxOccurs="30"
+ dfdl:lengthKind="explicit"
+ dfdl:length="16"
+ dfdl:occursCountKind="implicit"/>
+ <!-- Then this optional element is pulled. It will also fail due to the odd length of the layer. -->
+ <element name="postData" type="unsignedByte" minOccurs="0"/>
+ </sequence>
+ </complexType>
+ </element><!-- end block -->
+ </sequence><!-- end layer -->
+ <!-- this postLayer is after the empty layer
+ The layer -->
Review Comment:
typo? Should "The layer" be removed?
--
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