IGNITE-27431 SQL Calcite: Optimize scans with filter #12600
Conversation
alex-plekhanov
force-pushed
the
ignite-27431
branch
from
2994376 to
b516462
Compare
alex-plekhanov
force-pushed
the
ignite-27431
branch
from
b516462 to
96b2606
Compare
|
| rowType = desc.rowType(ectx.getTypeFactory(), requiredColumns); | ||
| factory = ectx.rowHandler().factory(ectx.getTypeFactory(), rowType); | ||
|
|
||
| ImmutableBitSet reqCols = requiredColumns == null ? ImmutableBitSet.range(0, rowType.getFieldCount()) |
There was a problem hiding this comment.
Minor codestyle. I would prefer:
ImmutableBitSet reqCols = requiredColumns == null
? ImmutableBitSet.range(0, rowType.getFieldCount())
: requiredColumns;
| /** | ||
| * Interface to iterate over raw table data and convert this data to relational node rows. | ||
| * | ||
| * @param <TableRow>> Raw table row type. |
There was a problem hiding this comment.
>> -> > . Two times, one below. Maybe single-line.
| public interface TableRowIterable<TableRow, Row> extends Iterable<Row> { | ||
| /** | ||
| * @return Table row iterator. | ||
| * */ |
| public Iterator<TableRow> tableRowIterator(); | ||
|
|
||
| /** | ||
| * Enriches {@code nodeRow} with columns from {@code tableRow} * |
There was a problem hiding this comment.
from {@code tableRow} * -> from {@code tableRow}.
| /** Participating columns. */ | ||
| protected final ImmutableBitSet requiredColumns; | ||
| /** Row field to column mapping. */ | ||
| protected final int[] fieldColMapping; |
There was a problem hiding this comment.
Suggestion: it looks bit confusing. Let's rename pameters and fields like this everywhere to something like:
rawToRelColMapping and/or relToRawColMapping
| * @param nodeRow Relational node row. | ||
| * @param fieldColMapping Mapping from node row fields to table row columns. If column is not requried | ||
| * corresponding field has -1 mapped value. | ||
| * @return Enriched relational node row. |
There was a problem hiding this comment.
If we return a relational row, why the mapping above is from node row to table row. The suggested renaming of fieldColMapping should also help with this.
| * @param <TableRow>> Raw table row type. | ||
| * @param <Row>> Relational node row type. | ||
| */ | ||
| public interface TableRowIterable<TableRow, Row> extends Iterable<Row> { |
There was a problem hiding this comment.
Do we need this new extra interface? Looks like we already have a bunch of scan interfaces and abstract classes. Can it be inside AbstractCacheColumnsScan?
| for (int i = 0; i < fieldColMapping.length; i++) { | ||
| // j = source field index, i = target field index. | ||
| int keyIdx = idxFieldMapping.indexOf(j); | ||
| int keyIdx = idxFieldMapping.indexOf(fieldColMapping[i]); |
There was a problem hiding this comment.
What is 'j' now in the comment above?
| * @param <TableRow>> Raw table row type. | ||
| * @param <Row>> Relational node row type. | ||
| */ | ||
| public interface TableRowIterable<TableRow, Row> extends Iterable<Row> { |
There was a problem hiding this comment.
Naming. If we convert, the name might be smth. like TableRawIterableConverter or TableRowFilterableIterable
|
@alex-plekhanov , could you please merge master? |
2 participants
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email [email protected] or ask anу advice on http://asf.slack.com #ignite channel.