-
Notifications
You must be signed in to change notification settings - Fork 108
GH-725: Added ExtensionReader #726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment has been minimized.
This comment has been minimized.
|
Thanks for the PR, let me take a look. |
|
@xxlaykxx sure. I will. |
|
still need review, plz |
| </#list></#list> | ||
|
|
||
| public void read(ExtensionHolder holder) { | ||
| fail("Extension"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be clearer to use the signature as the error message (like other methods do below)?
| public void read(ExtensionHolder holder) { | ||
| UuidHolder uuidHolder = (UuidHolder) holder; | ||
| vector.get(idx(), uuidHolder); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we need to override the other methods in AbstractFieldReader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
read(int arrayIndex, ExtensionHolder holder), copyAsValue, copyAsField still aren't implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added read, copyAsValue. copyAsField can't find any usage for other classes where this impl exists.
| FieldReader uuidReader = rootReader.reader("uuid1"); | ||
| uuidReader.setPosition(0); | ||
| UuidHolder uuidHolder = new UuidHolder(); | ||
| uuidReader.read(uuidHolder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly we test read but not the other methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
read(int arrayIndex, ExtensionHolder holder), copyAsValue, copyAsField aren't tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests with read and copyAsValue usage. For copyAsField, there are no examples of how it should be used, so not sure what is the right test case for it.
lidavidm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, LGTM
|
argh...I forgot to reapprove workflows...let me fix thigns |
ExtensionReader was added to support reading extension types from a complex vector. It contains **read(ExtensionHolder)** method for reading to the holder. And **readObject** - for reading the value explicitly. Closes apache#725.
ExtensionReader was added to support reading extension types from a complex vector. It contains **read(ExtensionHolder)** method for reading to the holder. And **readObject** - for reading the value explicitly. Closes apache#725.
Bumps [org.apache.commons:commons-text](https://github.com/apache/commons-text) from 1.13.1 to 1.15.0. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/apache/commons-text/blob/master/RELEASE-NOTES.txt">org.apache.commons:commons-text's changelog</a>.</em></p> <blockquote> <h2>Apache Commons Text 1.15.0 Release Notes</h2> <p>The Apache Commons Text team is pleased to announce the release of Apache Commons Text 1.15.0.</p> <p>Apache Commons Text is a set of utility functions and reusable components for processing and manipulating text in a Java environment.</p> <p>Release 1.15.0. This is a feature and maintenance release. Java 8 or later is required.</p> <h2>New features</h2> <ul> <li> <pre><code> Add experimental CycloneDX VEX file [#683](apache/commons-text#683). Thanks to Piotr P. Karwasz, Gary Gregory. </code></pre> </li> <li>TEXT-235: Add Damerau-Levenshtein distance <a href="https://redirect.github.com/apache/commons-text/issues/687">#687</a>. Thanks to LorgeN, Gary Gregory.</li> <li> <pre><code> Add unit tests to increase coverage [#719](apache/commons-text#719). Thanks to Michael Hausegger, Gary Gregory. </code></pre> </li> <li> <pre><code> Add new test for CharSequenceTranslator#with() [#725](apache/commons-text#725). Thanks to Michael Hausegger, Gary Gregory. </code></pre> </li> <li> <pre><code> Add tests and assertions to org.apache.commons.text.similarity to get to 100% code coverage [#727](apache/commons-text#727), [#728](apache/commons-text#728). Thanks to Michael Hausegger. </code></pre> </li> </ul> <h2>Fixed Bugs</h2> <ul> <li> <pre><code> Fix exception message typo in XmlStringLookup.XmlStringLookup(Map, Path...). Thanks to Gary Gregory. </code></pre> </li> <li>TEXT-236: Inserting at the end of a TextStringBuilder throws a StringIndexOutOfBoundsException. Thanks to Pierre Post, Sumit Bera, Alex Herbert, Gary Gregory.</li> <li> <pre><code> Fix TextStringBuilderTest.testAppendToCharBuffer() to use proper argument type [#724](apache/commons-text#724). Thanks to Michael Hausegger. </code></pre> </li> <li> <pre><code> Fix Apache RAT plugin console warnings. Thanks to Gary Gregory. </code></pre> </li> <li> <pre><code> Fix site XML to use version 2.0.0 XML schema. Thanks to Gary Gregory. </code></pre> </li> <li> <pre><code> Removed unreachable threshold verification code in src/main/java/org/apache/commons/text/similarity [#730](apache/commons-text#730). Thanks to Michael Hausegger. </code></pre> </li> <li> <pre><code> Enable secure processing for the XML parser in XmlStringLookup in case the underlying JAXP implementation doesn't [#729](apache/commons-text#729). Thanks to 김민재 (minjas0507), Gary Gregory, Piotr Karwasz. </code></pre> </li> </ul> <h2>Changes</h2> <ul> <li> <pre><code> Bump org.apache.commons:commons-parent from 85 to 93 [#704](apache/commons-text#704), [#723](apache/commons-text#723), [#726](apache/commons-text#726). Thanks to Gary Gregory. </code></pre> </li> <li> <pre><code> Bump commons.bytebuddy.version from 1.17.6 to 1.18.2 [#696](apache/commons-text#696), [#722](apache/commons-text#722). Thanks to Gary Gregory. </code></pre> </li> <li> <pre><code> Bump graalvm.version from 24.2.2 to 25.0.1 [#703](apache/commons-text#703), [#716](apache/commons-text#716). Thanks to Gary Gregory, Dependabot. </code></pre> </li> <li> <pre><code> Bump org.apache.commons:commons-lang3 from 3.18.0 to 3.20.0. Thanks to Gary Gregory. </code></pre> </li> <li> <pre><code> Bump commons-io:commons-io from 2.20.0 to 2.21.0. Thanks to Gary Gregory. </code></pre> </li> </ul> <p>Historical list of changes: <a href="https://commons.apache.org/proper/commons-text/changes.html">https://commons.apache.org/proper/commons-text/changes.html</a></p> <p>For complete information on Apache Commons Text, including instructions on how to submit bug reports, patches, or suggestions for improvement, see the Apache Commons Text website:</p> <p><a href="https://commons.apache.org/proper/commons-text">https://commons.apache.org/proper/commons-text</a></p> <p>Download page: <a href="https://commons.apache.org/proper/commons-text/download_text.cgi">https://commons.apache.org/proper/commons-text/download_text.cgi</a></p> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/apache/commons-text/commit/04e937470d3679cc163df85d82d5b6d2e3e71128"><code>04e9374</code></a> Prepare for the release candidate 1.15.0 RC1</li> <li><a href="https://github.com/apache/commons-text/commit/502c4c41be5671681b58a9b50297f99737e8ea93"><code>502c4c4</code></a> Prepare for the next release candidate</li> <li><a href="https://github.com/apache/commons-text/commit/c6e17ec24cc8374eb12676b717bf797f41b6e539"><code>c6e17ec</code></a> Use direct access</li> <li><a href="https://github.com/apache/commons-text/commit/58e1e125daaa0aebf8c5ffaa82af48821a1ccf2d"><code>58e1e12</code></a> Simplify XML FSP (<a href="https://redirect.github.com/apache/commons-text/issues/731">#731</a>)</li> <li><a href="https://github.com/apache/commons-text/commit/b5052c97e84e1c174ec8bfbbb749e33f22917a07"><code>b5052c9</code></a> Bump actions/setup-java from 5.0.0 to 5.1.0</li> <li><a href="https://github.com/apache/commons-text/commit/2e2d4bc90f1b3274e7943ac27d037d47c0cc098d"><code>2e2d4bc</code></a> Revert "Bump actions/setup-java from 5.0.0 to 5.1.0"</li> <li><a href="https://github.com/apache/commons-text/commit/b0ddbd17bbeee12ad33b8a61c60b4edbe6c85838"><code>b0ddbd1</code></a> Bump actions/setup-java from 5.0.0 to 5.1.0</li> <li><a href="https://github.com/apache/commons-text/commit/1c2d3821e67e08342b8cef4d4445c30b4a22daca"><code>1c2d382</code></a> Add tests with external DTD</li> <li><a href="https://github.com/apache/commons-text/commit/ed3df4b25cd5301921a6523ae7db2411f4a84d98"><code>ed3df4b</code></a> Internal clean up</li> <li><a href="https://github.com/apache/commons-text/commit/bb508f304a8835ac2319af1d872b2f1a9ff6f81d"><code>bb508f3</code></a> Bump actions/checkout from 6.0.0 to 6.0.1</li> <li>Additional commits viewable in <a href="https://github.com/apache/commons-text/compare/rel/commons-text-1.13.1...rel/commons-text-1.15.0">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
What's Changed
ExtensionReader was added to support reading extension types from a complex vector.
It contains read(ExtensionHolder) method for reading to the holder. And readObject - for reading the value explicitly.
Closes #725.