[compiler] Add SJavaString / SJavaBytes / SJavaArrayString #10625
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tpoterba
force-pushed
the
java-string
branch
2 times, most recently
from
96362ba to
69ef775
Compare
tpoterba
force-pushed
the
java-string
branch
2 times, most recently
from
360e172 to
19d1ea3
Compare
chrisvittal
previously requested changes
Jul 30, 2021
Comment on lines
+260
to
+269
| val v1 = cb.newLocal[String]("hamming_str_1", sc1.loadString()) | ||
| val v2 = cb.newLocal[String]("hamming_str_2", sc2.loadString()) | ||
|
|
||
| val m = v1.loadLength().cne(v2.loadLength()) | ||
| val l1 = cb.newLocal[Int]("hamming_len_1", v1.invoke[Int]("length")) | ||
| val l2 = cb.newLocal[Int]("hamming_len_2", v2.invoke[Int]("length")) | ||
| val m = l1.cne(l2) | ||
|
|
||
| IEmitCode(cb, m, { | ||
| cb.whileLoop(i < v1.loadLength(), { | ||
| cb.ifx(v1.loadByte(i).cne(v2.loadByte(i)), | ||
| cb.whileLoop(i < l1, { | ||
| cb.ifx(v1.invoke[Int, Char]("charAt", i).toI.cne(v2.invoke[Int, Char]("charAt", i).toI), |
There was a problem hiding this comment.
This is a behavioral change. The old version calculated the hamming distance on bytes this one does so on character sequences.
There was a problem hiding this comment.
I agree, the old implementation was wrong.
| @@ -19,7 +19,7 @@ object ShuffleOrdering { | |||
| val bcode1 = x.asInstanceOf[SCanonicalShufflePointerCode].binaryRepr | |||
| val bcode2 = y.asInstanceOf[SCanonicalShufflePointerCode].binaryRepr | |||
| val ord = BinaryOrdering.make(bcode1.st, bcode2.st, ecb) | |||
| ord.compareNonnull(cb, x.asString.asBytes(), y.asString.asBytes()) | |||
| ord.compareNonnull(cb, x.asInstanceOf[SCanonicalShufflePointerCode].binaryRepr, y.asInstanceOf[SCanonicalShufflePointerCode].binaryRepr) | |||
There was a problem hiding this comment.
can't this just use bcode1 and bcode2?
Comment on lines
47
to
48
| private[this] def memoizeWithBuilder(cb: EmitCodeBuilder, name: String, sb: SettableBuilder): SBinaryValue = { | ||
| val s = new SJavaBytesSettable(sb.newSettable[Array[Byte]]("sjavabytes_memoize")) |
There was a problem hiding this comment.
name is unused, maybe need to use SJavaBytesSettable.apply?
There was a problem hiding this comment.
just need to use name in the string passed here.
Comment on lines
59
to
60
| private[this] def memoizeWithBuilder(cb: EmitCodeBuilder, name: String, sb: SettableBuilder): SValue = { | ||
| val s = new SJavaStringSettable(sb.newSettable[String]("sjavastring_memoize")) |
There was a problem hiding this comment.
name unused, same as SJavaBytes comment
| case _ => | ||
| val sv = code.asIndexable.memoize(cb, "scode_array_string") | ||
| val arr = cb.newLocal[Array[String]]("scode_array_string", Code.newArray[String](sv.loadLength())) | ||
| sv.foreach(cb) { case (cb, idx, elt) => |
There was a problem hiding this comment.
why do you need this new foreach method rather than foreachDefined here?
There was a problem hiding this comment.
ah, that will work.
chrisvittal
approved these changes
Jul 30, 2021
daniel-goldstein
pushed a commit
to daniel-goldstein/hail
that referenced
this pull request
Aug 3, 2021
…0625) * [compiler] Add SJavaString / SJavaBytes / SJavaArrayString * some fixes * fix bytes * add check * fix array wrap * fix java array virtual type and settable tuple types * fix arraylen * fix rg * addressed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.