ascherbakoff commented on a change in pull request #168:
URL: https://github.com/apache/ignite-3/pull/168#discussion_r655978177
##########
File path:
modules/schema/src/main/java/org/apache/ignite/internal/schema/row/Row.java
##########
@@ -343,65 +355,55 @@ private boolean hasFlag(int flag) {
*/
protected long findColumn(int colIdx, NativeTypeSpec type) throws
InvalidTypeException {
// Get base offset (key start or value start) for the given column.
- boolean keyCol = schema.isKeyColumn(colIdx);
-
- final short flags = readShort(FLAGS_FIELD_OFFSET);
+ boolean isKeyCol = schema.isKeyColumn(colIdx);
- int off = KEY_CHUNK_OFFSET;
-
- if (!keyCol) {
- assert (flags & RowFlags.NO_VALUE_FLAG) == 0;
-
- // Jump to the next chunk, the size of the first chunk is written
at the chunk start.
- off += readInteger(off);
-
- // Adjust the column index according to the number of key columns.
+ // Adjust the column index according to the number of key columns.
+ if (!isKeyCol)
colIdx -= schema.keyColumns().length();
- }
- Columns cols = keyCol ? schema.keyColumns() : schema.valueColumns();
+ ChunkReader reader = isKeyCol ? keyReader : valueReader();
+ Columns cols = isKeyCol ? schema.keyColumns() : schema.valueColumns();
if (cols.column(colIdx).type().spec() != type)
throw new InvalidTypeException("Invalid column type requested
[requested=" + type +
", column=" + cols.column(colIdx) + ']');
- boolean hasVarTable = ((keyCol ? RowFlags.OMIT_KEY_VARTBL_FLAG :
RowFlags.OMIT_VAL_VARTBL_FLAG) & flags) == 0;
- boolean hasNullMap = ((keyCol ? RowFlags.OMIT_KEY_NULL_MAP_FLAG :
RowFlags.OMIT_VAL_NULL_MAP_FLAG) & flags) == 0;
+ assert reader != null;
- if (hasNullMap && isNull(off, colIdx))
+ if (reader.isNull(colIdx))
return -1;
return type.fixedLength() ?
- fixlenColumnOffset(cols, off, colIdx, hasVarTable, hasNullMap) :
- varlenColumnOffsetAndLength(cols, off, colIdx, hasVarTable,
hasNullMap);
+ reader.fixlenColumnOffset(cols, colIdx) :
Review comment:
It seems to me the reader should not implement these methods. Instread,
it should be passed as an argument to the corresponding methods for offset
calculation.
##########
File path:
modules/schema/src/main/java/org/apache/ignite/internal/schema/row/Row.java
##########
@@ -343,65 +355,55 @@ private boolean hasFlag(int flag) {
*/
protected long findColumn(int colIdx, NativeTypeSpec type) throws
InvalidTypeException {
// Get base offset (key start or value start) for the given column.
- boolean keyCol = schema.isKeyColumn(colIdx);
-
- final short flags = readShort(FLAGS_FIELD_OFFSET);
+ boolean isKeyCol = schema.isKeyColumn(colIdx);
- int off = KEY_CHUNK_OFFSET;
-
- if (!keyCol) {
- assert (flags & RowFlags.NO_VALUE_FLAG) == 0;
-
- // Jump to the next chunk, the size of the first chunk is written
at the chunk start.
- off += readInteger(off);
-
- // Adjust the column index according to the number of key columns.
+ // Adjust the column index according to the number of key columns.
+ if (!isKeyCol)
colIdx -= schema.keyColumns().length();
- }
- Columns cols = keyCol ? schema.keyColumns() : schema.valueColumns();
+ ChunkReader reader = isKeyCol ? keyReader : valueReader();
+ Columns cols = isKeyCol ? schema.keyColumns() : schema.valueColumns();
if (cols.column(colIdx).type().spec() != type)
throw new InvalidTypeException("Invalid column type requested
[requested=" + type +
", column=" + cols.column(colIdx) + ']');
- boolean hasVarTable = ((keyCol ? RowFlags.OMIT_KEY_VARTBL_FLAG :
RowFlags.OMIT_VAL_VARTBL_FLAG) & flags) == 0;
- boolean hasNullMap = ((keyCol ? RowFlags.OMIT_KEY_NULL_MAP_FLAG :
RowFlags.OMIT_VAL_NULL_MAP_FLAG) & flags) == 0;
+ assert reader != null;
- if (hasNullMap && isNull(off, colIdx))
+ if (reader.isNull(colIdx))
return -1;
return type.fixedLength() ?
- fixlenColumnOffset(cols, off, colIdx, hasVarTable, hasNullMap) :
- varlenColumnOffsetAndLength(cols, off, colIdx, hasVarTable,
hasNullMap);
+ reader.fixlenColumnOffset(cols, colIdx) :
Review comment:
It seems to me the reader should not implement these methods. Instread,
it should be makde stateless abd passed as an argument to the corresponding
methods for offset calculation. This can help to avoid unnecessary object
creation.
##########
File path:
modules/schema/src/main/java/org/apache/ignite/internal/schema/row/Row.java
##########
@@ -343,65 +355,55 @@ private boolean hasFlag(int flag) {
*/
protected long findColumn(int colIdx, NativeTypeSpec type) throws
InvalidTypeException {
// Get base offset (key start or value start) for the given column.
- boolean keyCol = schema.isKeyColumn(colIdx);
-
- final short flags = readShort(FLAGS_FIELD_OFFSET);
+ boolean isKeyCol = schema.isKeyColumn(colIdx);
- int off = KEY_CHUNK_OFFSET;
-
- if (!keyCol) {
- assert (flags & RowFlags.NO_VALUE_FLAG) == 0;
-
- // Jump to the next chunk, the size of the first chunk is written
at the chunk start.
- off += readInteger(off);
-
- // Adjust the column index according to the number of key columns.
+ // Adjust the column index according to the number of key columns.
+ if (!isKeyCol)
colIdx -= schema.keyColumns().length();
- }
- Columns cols = keyCol ? schema.keyColumns() : schema.valueColumns();
+ ChunkReader reader = isKeyCol ? keyReader : valueReader();
+ Columns cols = isKeyCol ? schema.keyColumns() : schema.valueColumns();
if (cols.column(colIdx).type().spec() != type)
throw new InvalidTypeException("Invalid column type requested
[requested=" + type +
", column=" + cols.column(colIdx) + ']');
- boolean hasVarTable = ((keyCol ? RowFlags.OMIT_KEY_VARTBL_FLAG :
RowFlags.OMIT_VAL_VARTBL_FLAG) & flags) == 0;
- boolean hasNullMap = ((keyCol ? RowFlags.OMIT_KEY_NULL_MAP_FLAG :
RowFlags.OMIT_VAL_NULL_MAP_FLAG) & flags) == 0;
+ assert reader != null;
- if (hasNullMap && isNull(off, colIdx))
+ if (reader.isNull(colIdx))
return -1;
return type.fixedLength() ?
- fixlenColumnOffset(cols, off, colIdx, hasVarTable, hasNullMap) :
- varlenColumnOffsetAndLength(cols, off, colIdx, hasVarTable,
hasNullMap);
+ reader.fixlenColumnOffset(cols, colIdx) :
Review comment:
It seems to me the reader should not implement these methods. Instead,
it should be made stateless and passed as an argument to the corresponding
methods for offset calculation. This can help to avoid unnecessary object
creation.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
