You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by "leerho (via GitHub)" <gi...@apache.org> on 2023/06/13 22:27:31 UTC
[GitHub] [datasketches-java] leerho opened a new pull request, #453: Update theta compact() behavior.
leerho opened a new pull request, #453:
URL: https://github.com/apache/datasketches-java/pull/453
Update associated javadocs.
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228821508
##########
src/main/java/org/apache/datasketches/theta/Sketch.java:
##########
@@ -193,43 +193,50 @@ public static Sketch wrap(final Memory srcMem, final long expectedSeed) {
//Sketch interface
/**
- * Converts this sketch to a ordered CompactSketch on the Java heap.
+ * Converts this sketch to a ordered CompactSketch.
*
- * <p>If this sketch is already in the proper form, this method returns <i>this</i>,
- * otherwise, this method returns a new CompactSketch of the proper form.
+ * <p>If <i>this.isCompact() == true</i> this method returns <i>this</i>,
Review Comment:
I'm willing to change it **here** but your comments don't give me a clue whether to use `<em>` or `<tt>`.
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228823678
##########
src/main/java/org/apache/datasketches/theta/Sketch.java:
##########
@@ -193,43 +193,50 @@ public static Sketch wrap(final Memory srcMem, final long expectedSeed) {
//Sketch interface
/**
- * Converts this sketch to a ordered CompactSketch on the Java heap.
+ * Converts this sketch to a ordered CompactSketch.
*
- * <p>If this sketch is already in the proper form, this method returns <i>this</i>,
- * otherwise, this method returns a new CompactSketch of the proper form.
+ * <p>If <i>this.isCompact() == true</i> this method returns <i>this</i>,
Review Comment:
Besides, the <tt> tag is deprecated and no longer recommended: [The Teletype Text element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/tt)
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228823678
##########
src/main/java/org/apache/datasketches/theta/Sketch.java:
##########
@@ -193,43 +193,50 @@ public static Sketch wrap(final Memory srcMem, final long expectedSeed) {
//Sketch interface
/**
- * Converts this sketch to a ordered CompactSketch on the Java heap.
+ * Converts this sketch to a ordered CompactSketch.
*
- * <p>If this sketch is already in the proper form, this method returns <i>this</i>,
- * otherwise, this method returns a new CompactSketch of the proper form.
+ * <p>If <i>this.isCompact() == true</i> this method returns <i>this</i>,
Review Comment:
Besides, the \<tt\> tag is deprecated and no longer recommended: [The Teletype Text element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/tt)
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] jmalkin commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228824384
##########
src/main/java/org/apache/datasketches/theta/Sketch.java:
##########
@@ -193,43 +193,50 @@ public static Sketch wrap(final Memory srcMem, final long expectedSeed) {
//Sketch interface
/**
- * Converts this sketch to a ordered CompactSketch on the Java heap.
+ * Converts this sketch to a ordered CompactSketch.
*
- * <p>If this sketch is already in the proper form, this method returns <i>this</i>,
- * otherwise, this method returns a new CompactSketch of the proper form.
+ * <p>If <i>this.isCompact() == true</i> this method returns <i>this</i>,
Review Comment:
Oops.. ok, so then we should use the `<code>` replacement :)
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] jmalkin commented on pull request #453: Update theta compact() behavior.
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#issuecomment-1590188226
> Do we have to have compact() method in the compact theta sketch class? I would think it should exist in the updatable sketch only.
`compact()` is defined in the top-level Sketch interface so it'd be a major version bump to change that.
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] AlexanderSaydakov commented on pull request #453: Update theta compact() behavior.
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#issuecomment-1590183137
Do we have to have compact() method in the compact theta sketch class? I would think it should exist in the updatable sketch only.
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on pull request #453: Update theta compact() behavior.
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#issuecomment-1590254488
What changes are required to approve this? So far, I'll agree to the spelling 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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] jmalkin commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228797164
##########
src/main/java/org/apache/datasketches/theta/Sketch.java:
##########
@@ -193,43 +193,50 @@ public static Sketch wrap(final Memory srcMem, final long expectedSeed) {
//Sketch interface
/**
- * Converts this sketch to a ordered CompactSketch on the Java heap.
+ * Converts this sketch to a ordered CompactSketch.
*
- * <p>If this sketch is already in the proper form, this method returns <i>this</i>,
- * otherwise, this method returns a new CompactSketch of the proper form.
+ * <p>If <i>this.isCompact() == true</i> this method returns <i>this</i>,
Review Comment:
Two things here.
1) I think <em> is generally preferred to <i> in that it's semantic rather than direct typesetting. What does italics mean if using a screen reader, for instance?
2) For code snippets that aren't put into their own preformatted block, I think <tt> is generally preferred over italicized text.
That's going to apply to a _lot_ of code in this repo, though, so perhaps better suited to a dedicated PR that does only 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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] jmalkin commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228839993
##########
src/main/java/org/apache/datasketches/theta/Sketch.java:
##########
@@ -193,43 +193,50 @@ public static Sketch wrap(final Memory srcMem, final long expectedSeed) {
//Sketch interface
/**
- * Converts this sketch to a ordered CompactSketch on the Java heap.
+ * Converts this sketch to a ordered CompactSketch.
*
- * <p>If this sketch is already in the proper form, this method returns <i>this</i>,
- * otherwise, this method returns a new CompactSketch of the proper form.
+ * <p>If <i>this.isCompact() == true</i> this method returns <i>this</i>,
Review Comment:
Yes, we should definitely make that change. But as I said earlier, I think it makes more sense to do it as dedicated PRs per sketch family.
I'll resolve this for now since the concept has now been discussed.
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228832310
##########
src/main/java/org/apache/datasketches/theta/Sketch.java:
##########
@@ -193,43 +193,50 @@ public static Sketch wrap(final Memory srcMem, final long expectedSeed) {
//Sketch interface
/**
- * Converts this sketch to a ordered CompactSketch on the Java heap.
+ * Converts this sketch to a ordered CompactSketch.
*
- * <p>If this sketch is already in the proper form, this method returns <i>this</i>,
- * otherwise, this method returns a new CompactSketch of the proper form.
+ * <p>If <i>this.isCompact() == true</i> this method returns <i>this</i>,
Review Comment:
It is not so clear that \<em\> is necessarily the correct answer either. Even though Java documentation does use it.
"The \<i\> element represents text that is set off from the normal prose, such as a foreign word, fictional character thoughts, or when the text refers to the definition of a word instead of representing its semantic meaning."
"An example for \<em\> could be: "Just _do_ it already!", or: "We _had_ to do something about it". A person or software reading the text would pronounce the words in italics with an emphasis, using verbal stress." [The emphasis element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/em)
I don't think someone reading these javadoc aloud would use verbal stress on these words.
Is it such a big deal?
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho merged pull request #453: Update theta compact() behavior.
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho merged PR #453:
URL: https://github.com/apache/datasketches-java/pull/453
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] jmalkin commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228795279
##########
src/test/java/org/apache/datasketches/theta/CompactSketchTest.java:
##########
@@ -220,81 +221,226 @@ public void checkMemTooSmallOrdered() {
@Test
public void checkCompactCachePart() {
- //phony values except for curCount = 0.
+ //phoney values except for curCount = 0.
Review Comment:
We can switch to UK spellings but why only this one place in an internal comment?
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228838393
##########
src/main/java/org/apache/datasketches/theta/Sketch.java:
##########
@@ -193,43 +193,50 @@ public static Sketch wrap(final Memory srcMem, final long expectedSeed) {
//Sketch interface
/**
- * Converts this sketch to a ordered CompactSketch on the Java heap.
+ * Converts this sketch to a ordered CompactSketch.
*
- * <p>If this sketch is already in the proper form, this method returns <i>this</i>,
- * otherwise, this method returns a new CompactSketch of the proper form.
+ * <p>If <i>this.isCompact() == true</i> this method returns <i>this</i>,
Review Comment:
The [Oracle Style Guide](https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#styleguide) for Javadocs recommends using \<code\>...\</code\> for
- Java keywords
- package names
- class names
- method names
- interface names
- field names
- argument names
- code examples
This would represent a big cleanup of the entire repo if we were to decide to do this.
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on pull request #453: Update theta compact() behavior.
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#issuecomment-1590176116
Will noticed that when calling compact() on a Theta CompactSketch, instead of just returning `this` the code created another compact object. After digging into it, I realized that not only was the code not optimal, it was not well documented either. The test code also needed to be rewritten to reflect this updated behavior. This PR fixes these issues.
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] will-lauer commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "will-lauer (via GitHub)" <gi...@apache.org>.
will-lauer commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1229893283
##########
src/main/java/org/apache/datasketches/theta/Sketch.java:
##########
@@ -193,43 +193,50 @@ public static Sketch wrap(final Memory srcMem, final long expectedSeed) {
//Sketch interface
/**
- * Converts this sketch to a ordered CompactSketch on the Java heap.
+ * Converts this sketch to a ordered CompactSketch.
*
- * <p>If this sketch is already in the proper form, this method returns <i>this</i>,
- * otherwise, this method returns a new CompactSketch of the proper form.
+ * <p>If <i>this.isCompact() == true</i> this method returns <i>this</i>,
Review Comment:
I know I'm late to this converstation, but don't forget about the `{@code}` annotation in javadoc. This is also a possibility instead of `<code>` and definitely preferable to things like `<em>` and `<i>`
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] jmalkin commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228797164
##########
src/main/java/org/apache/datasketches/theta/Sketch.java:
##########
@@ -193,43 +193,50 @@ public static Sketch wrap(final Memory srcMem, final long expectedSeed) {
//Sketch interface
/**
- * Converts this sketch to a ordered CompactSketch on the Java heap.
+ * Converts this sketch to a ordered CompactSketch.
*
- * <p>If this sketch is already in the proper form, this method returns <i>this</i>,
- * otherwise, this method returns a new CompactSketch of the proper form.
+ * <p>If <i>this.isCompact() == true</i> this method returns <i>this</i>,
Review Comment:
Two things here.
1) I think `<em>` is generally preferred to `<i>` in that it's semantic rather than direct typesetting. What does italics mean if using a screen reader, for instance?
2) For code snippets that aren't put into their own preformatted block, I think `<tt>` is generally preferred over italicized text.
That's going to apply to a huge amount of javadocs in this repo, though, so perhaps better suited to a dedicated PR that does only 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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] jmalkin commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228824069
##########
src/main/java/org/apache/datasketches/theta/Sketch.java:
##########
@@ -193,43 +193,50 @@ public static Sketch wrap(final Memory srcMem, final long expectedSeed) {
//Sketch interface
/**
- * Converts this sketch to a ordered CompactSketch on the Java heap.
+ * Converts this sketch to a ordered CompactSketch.
*
- * <p>If this sketch is already in the proper form, this method returns <i>this</i>,
- * otherwise, this method returns a new CompactSketch of the proper form.
+ * <p>If <i>this.isCompact() == true</i> this method returns <i>this</i>,
Review Comment:
Hoping I can get a comment from someone else. I think I'd vote for `<tt>` -- but I was more using it as a discussion point since I noticed here. If we're going to change the style we should try to do this everywhere in a single pass, IMHO. Or at least a series of PRs per sketch type/family.
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on a diff in pull request #453: Update theta compact() behavior.
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #453:
URL: https://github.com/apache/datasketches-java/pull/453#discussion_r1228821834
##########
src/test/java/org/apache/datasketches/theta/CompactSketchTest.java:
##########
@@ -220,81 +221,226 @@ public void checkMemTooSmallOrdered() {
@Test
public void checkCompactCachePart() {
- //phony values except for curCount = 0.
+ //phoney values except for curCount = 0.
Review Comment:
Funny, when I did my spellcheck it suggested "phoney" I didn't give it a second glance and realize that this is the British spelling :(
--
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@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org