korlov42 commented on code in PR #5536: URL: https://github.com/apache/ignite-3/pull/5536#discussion_r2028242283
########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/UpgradeRowBenchmark.java: ########## @@ -0,0 +1,115 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.benchmark; + +import static org.apache.ignite.internal.lang.IgniteStringFormatter.format; + +import java.io.IOException; +import java.util.concurrent.TimeUnit; +import org.apache.ignite.sql.IgniteSql; +import org.apache.ignite.sql.ResultSet; +import org.apache.ignite.sql.SqlRow; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +/** + * Benchmark that runs sql queries via embedded client on clusters of different size. + */ +@State(Scope.Benchmark) +@Fork(1) +@Threads(1) +@Warmup(iterations = 10, time = 2) +@Measurement(iterations = 20, time = 2) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.MILLISECONDS) +@SuppressWarnings({"WeakerAccess", "unused"}) +public class UpgradeRowBenchmark extends AbstractMultiNodeBenchmark { + private static final int TABLE_SIZE = 300_000; + + private IgniteSql sql; + + @Param({"1"}) + private int clusterSize; + + /** Fills the table with data. */ + @Setup + public void setUp() throws IOException { + populateTable(TABLE_NAME, TABLE_SIZE, 1_000); + + sql = publicIgnite.sql(); + + try (ResultSet<SqlRow> rs = sql.execute(null, format("ALTER TABLE {} ADD COLUMN new_col INT DEFAULT 42", TABLE_NAME));) { + while (rs.hasNext()) { + rs.next(); + } + } + } + + /** Benchmark that measures performance of `SELECT *` query over entire table. */ + @Benchmark + public void selectAll(Blackhole bh) { + try (var rs = sql.execute(null, "SELECT * FROM usertable")) { + while (rs.hasNext()) { + bh.consume(rs.next()); + } + } + } + + /** Benchmark that measures performance of `SELECT` query with projection over entire table. */ + @Benchmark + public void selectAllProjected(Blackhole bh) { + try (var rs = sql.execute(null, "SELECT field1, field2, field3 FROM usertable")) { + while (rs.hasNext()) { + bh.consume(rs.next()); + } + } + } + + /** + * Benchmark's entry point. + */ + public static void main(String[] args) throws RunnerException { + Options opt = new OptionsBuilder() + .include(".*" + UpgradeRowBenchmark.class.getSimpleName() + ".*") + .build(); + + new Runner(opt).run(); + } + + @Override + protected int nodes() { + return clusterSize; + } +} + + Review Comment: extra line ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/ProjectedTuple.java: ########## @@ -17,26 +17,22 @@ package org.apache.ignite.internal.sql.engine.util; -import java.nio.ByteBuffer; import org.apache.ignite.internal.binarytuple.BinaryTupleBuilder; -import org.apache.ignite.internal.binarytuple.BinaryTupleParser; import org.apache.ignite.internal.binarytuple.BinaryTupleParser.Sink; import org.apache.ignite.internal.lang.InternalTuple; import org.apache.ignite.internal.schema.BinaryTuple; +import org.apache.ignite.internal.schema.InternalTupleEx; /** - * A projected tuple that aware of the format of delegate. - * - * <p>That is, the format of delegate is known to be Binary Tuple, thus it's possible to avoid unnecessary - * (de-)serialization during tuple normalization. + * A projected tuple wrapper that is best effort to avoiding unnecessary (de-)serialization during tuple normalization. * * <p>It's up to the caller to get sure that provided tuple respect the format. Review Comment: this line is no longer valid, let's remove it ########## modules/binary-tuple/src/main/java/org/apache/ignite/internal/binarytuple/BinaryTupleParser.java: ########## @@ -504,6 +504,22 @@ public final Period periodValue(int begin, int end) { } } + /** + * Copies raw value of the specified element to the builder. + * + * @param builder Builder to copy value to. + * @param index Index of the element. + */ + protected void copyRawValue(BinaryTupleBuilder builder, int index) { + fetch(index, (idx, begin, end) -> { Review Comment: I think we can avoid creation of lambda on every read by moving this method to `BinaryTupleReader`. As downside we will need to make buffer package-private. WDYT? ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/ProjectedTuple.java: ########## @@ -45,49 +41,47 @@ public class FormatAwareProjectedTuple extends AbstractProjectedTuple { * an index of field in resulting projection, and an element of the array at that index is an index of column in original * tuple. */ - public FormatAwareProjectedTuple(InternalTuple delegate, int[] projection) { - super(delegate, projection); + public ProjectedTuple(InternalTuple delegate, int[] projection) { + super((InternalTupleEx) delegate, projection); Review Comment: tbh I don't really like this approach with force casting. Let's try to preserve type safety ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/ProjectedTuple.java: ########## @@ -17,26 +17,22 @@ package org.apache.ignite.internal.sql.engine.util; -import java.nio.ByteBuffer; import org.apache.ignite.internal.binarytuple.BinaryTupleBuilder; -import org.apache.ignite.internal.binarytuple.BinaryTupleParser; import org.apache.ignite.internal.binarytuple.BinaryTupleParser.Sink; import org.apache.ignite.internal.lang.InternalTuple; import org.apache.ignite.internal.schema.BinaryTuple; +import org.apache.ignite.internal.schema.InternalTupleEx; /** - * A projected tuple that aware of the format of delegate. - * - * <p>That is, the format of delegate is known to be Binary Tuple, thus it's possible to avoid unnecessary - * (de-)serialization during tuple normalization. + * A projected tuple wrapper that is best effort to avoiding unnecessary (de-)serialization during tuple normalization. * * <p>It's up to the caller to get sure that provided tuple respect the format. * * <p>Not thread safe! * * @see AbstractProjectedTuple */ -public class FormatAwareProjectedTuple extends AbstractProjectedTuple { +public class ProjectedTuple extends AbstractProjectedTuple { Review Comment: probably we should merge `ProjectedTuple` and `AbstractProjectedTuple` altogether. So we will have only `ProjectedTuple` and extension which append virtual columns. WDYT? -- 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-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
