You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ademakov (via GitHub)" <gi...@apache.org> on 2023/06/26 13:37:00 UTC
[GitHub] [ignite-3] ademakov opened a new pull request, #2256: IGNITE-19666 Remove nullmaps from binary tuples in C++
ademakov opened a new pull request, #2256:
URL: https://github.com/apache/ignite-3/pull/2256
(no 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #2256: IGNITE-19666 Remove nullmaps from binary tuples
Posted by "SammyVimes (via GitHub)" <gi...@apache.org>.
SammyVimes commented on code in PR #2256:
URL: https://github.com/apache/ignite-3/pull/2256#discussion_r1247849982
##########
modules/binary-tuple/src/main/java/org/apache/ignite/internal/binarytuple/BinaryTupleParser.java:
##########
@@ -551,7 +512,16 @@ private int getOffset(int index) {
}
/**
- * Decode a non-trivial Date element.
+ * Gets array of bytes from a given range in the buffer.
+ */
+ private byte[] getBytes(int begin, int end) {
+ byte[] bytes = new byte[end - begin];
+ buffer.duplicate().position(begin).limit(end).get(bytes);
Review Comment:
Oh, sorry
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #2256: IGNITE-19666 Remove nullmaps from binary tuples
Posted by "SammyVimes (via GitHub)" <gi...@apache.org>.
SammyVimes commented on code in PR #2256:
URL: https://github.com/apache/ignite-3/pull/2256#discussion_r1247497159
##########
modules/binary-tuple/src/main/java/org/apache/ignite/internal/binarytuple/BinaryTupleBuilder.java:
##########
@@ -754,8 +669,26 @@ private void putBytes(byte[] bytes) {
buffer.put(bytes);
}
+ private void putBytesWithEmptyCheck(byte[] bytes) {
+ if (bytes.length == 0 || bytes[0] == BinaryTupleCommon.VARLEN_EMPTY_BYTE) {
+ ensure(bytes.length + 1);
Review Comment:
why not just ensure(1) and put VARLEN_EMPTY_BYTE?
##########
modules/binary-tuple/src/main/java/org/apache/ignite/internal/binarytuple/BinaryTupleParser.java:
##########
@@ -551,7 +512,16 @@ private int getOffset(int index) {
}
/**
- * Decode a non-trivial Date element.
+ * Gets array of bytes from a given range in the buffer.
+ */
+ private byte[] getBytes(int begin, int end) {
+ byte[] bytes = new byte[end - begin];
+ buffer.duplicate().position(begin).limit(end).get(bytes);
Review Comment:
```suggestion
buffer.get(begin, bytes);
```
should be ok doing something like 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #2256: IGNITE-19666 Remove nullmaps from binary tuples
Posted by "SammyVimes (via GitHub)" <gi...@apache.org>.
SammyVimes commented on code in PR #2256:
URL: https://github.com/apache/ignite-3/pull/2256#discussion_r1247850508
##########
modules/binary-tuple/src/main/java/org/apache/ignite/internal/binarytuple/BinaryTupleBuilder.java:
##########
@@ -754,8 +669,26 @@ private void putBytes(byte[] bytes) {
buffer.put(bytes);
}
+ private void putBytesWithEmptyCheck(byte[] bytes) {
+ if (bytes.length == 0 || bytes[0] == BinaryTupleCommon.VARLEN_EMPTY_BYTE) {
+ ensure(bytes.length + 1);
Review Comment:
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ptupitsyn commented on pull request #2256: IGNITE-19666 Remove nullmaps from binary tuples
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on PR #2256:
URL: https://github.com/apache/ignite-3/pull/2256#issuecomment-1617275595
@SammyVimes please make sure to include extended commit message details when merging PRs:
1. Copy the PR description (if there is anything meaningful).
2. Include `Co-authored-by: NAME <NA...@EXAMPLE.COM>` when there are multiple authors - GitHub generates those lines automatically I believe.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes merged pull request #2256: IGNITE-19666 Remove nullmaps from binary tuples
Posted by "SammyVimes (via GitHub)" <gi...@apache.org>.
SammyVimes merged PR #2256:
URL: https://github.com/apache/ignite-3/pull/2256
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] isapego commented on a diff in pull request #2256: IGNITE-19666 Remove nullmaps from binary tuples
Posted by "isapego (via GitHub)" <gi...@apache.org>.
isapego commented on code in PR #2256:
URL: https://github.com/apache/ignite-3/pull/2256#discussion_r1247578481
##########
modules/platforms/cpp/ignite/tuple/binary_tuple_builder.cpp:
##########
@@ -66,44 +66,39 @@ binary_tuple_builder::binary_tuple_builder(tuple_num_t element_count) noexcept
void binary_tuple_builder::start() noexcept {
element_index = 0;
- null_elements = 0;
value_area_size = 0;
entry_size = 0;
}
void binary_tuple_builder::layout() {
+ using namespace ignite::binary_tuple_common;
+
assert(element_index == element_count);
binary_tuple_common::header header;
- size_t nullmapSize = 0;
- if (null_elements) {
- header.set_nullmap_flag();
- nullmapSize = binary_tuple_common::get_nullmap_size(element_count);
- }
-
entry_size = header.set_entry_size(value_area_size);
std::size_t tableSize = entry_size * element_count;
Review Comment:
```suggestion
std::size_t table_size = entry_size * element_count;
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ademakov commented on a diff in pull request #2256: IGNITE-19666 Remove nullmaps from binary tuples
Posted by "ademakov (via GitHub)" <gi...@apache.org>.
ademakov commented on code in PR #2256:
URL: https://github.com/apache/ignite-3/pull/2256#discussion_r1247754224
##########
modules/binary-tuple/src/main/java/org/apache/ignite/internal/binarytuple/BinaryTupleBuilder.java:
##########
@@ -754,8 +669,26 @@ private void putBytes(byte[] bytes) {
buffer.put(bytes);
}
+ private void putBytesWithEmptyCheck(byte[] bytes) {
+ if (bytes.length == 0 || bytes[0] == BinaryTupleCommon.VARLEN_EMPTY_BYTE) {
+ ensure(bytes.length + 1);
Review Comment:
Out of bad habit to count every instruction. This saves one comparison.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ademakov commented on a diff in pull request #2256: IGNITE-19666 Remove nullmaps from binary tuples
Posted by "ademakov (via GitHub)" <gi...@apache.org>.
ademakov commented on code in PR #2256:
URL: https://github.com/apache/ignite-3/pull/2256#discussion_r1247745262
##########
modules/binary-tuple/src/main/java/org/apache/ignite/internal/binarytuple/BinaryTupleParser.java:
##########
@@ -551,7 +512,16 @@ private int getOffset(int index) {
}
/**
- * Decode a non-trivial Date element.
+ * Gets array of bytes from a given range in the buffer.
+ */
+ private byte[] getBytes(int begin, int end) {
+ byte[] bytes = new byte[end - begin];
+ buffer.duplicate().position(begin).limit(end).get(bytes);
Review Comment:
This is available only since jdk13, for jdk11 it won't work.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org