ronantakizawa/codereview-bench huggingface dataset:
Repository: apache/kafka
File: tests/kafkatest/version.py (Python)

Code before:
V_3_7_2 = KafkaVersion("3.7.2")
LATEST_3_7 = V_3_7_2

# 3.8.x version
V_3_8_0 = KafkaVersion("3.8.0")
V_3_8_1 = KafkaVersion("3.8.1")
LATEST_3_8 = V_3_8_1

# 3.9.x version
V_3_9_0 = KafkaVersion("3.9.0")
V_3_9_1 = KafkaVersion("3.9.1")
LATEST_3_9 = V_3_9_1

# 4.0.x version
V_4_0_0 = KafkaVersion("4.0.0")
LATEST_4_0 = V_4_0_0

# 4.1.x version
V_4_1_0 = KafkaVersion("4.1.0")
V_4_1_1 = KafkaVersion("4.1.1")
LATEST_4_1 = V_4_1_1

# 4.2.x version
V_4_2_0 = KafkaVersion("4.2.0")
V_4_2_1 = KafkaVersion("4.2.1")
LATEST_4_2 = V_4_2_1

# 4.3.x version
V_4_3_0 = KafkaVersion("4.3.0")
LATEST_4_3 = V_4_3_0


Review comment (suggestion):
This seems not to be correct... We use `LATEST_4_2` in system tests, but `4.2.1` is no release yet, so this might break system test trying to test `4.2.0` is they only specify `LATEST_4_2` (what most system test do...)

And for `trunk` (in contrast to `4.2` branch), after 4.2.0 was release, we are going to use `LATEST_4_2`, and it must point to `4.2.0` not to unreleased `4.2.1` to work as expected, and not break system tests.

Code after:
V_3_7_2 = KafkaVersion("3.7.2")
LATEST_3_7 = V_3_7_2

# 3.8.x version
V_3_8_0 = KafkaVersion("3.8.0")
V_3_8_1 = KafkaVersion("3.8.1")
LATEST_3_8 = V_3_8_1

# 3.9.x version
V_3_9_0 = KafkaVersion("3.9.0")
V_3_9_1 = KafkaVersion("3.9.1")
LATEST_3_9 = V_3_9_1

# 4.0.x version
V_4_0_0 = KafkaVersion("4.0.0")
LATEST_4_0 = V_4_0_0

# 4.1.x version
V_4_1_0 = KafkaVersion("4.1.0")
V_4_1_1 = KafkaVersion("4.1.1")
LATEST_4_1 = V_4_1_1

# 4.2.x version
V_4_2_0 = KafkaVersion("4.2.0")
LATEST_4_2 = V_4_2_0
Repository: apache/kafka
File: core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala (Scala)

Code before:
    props ++= adminClientConfig
    props ++= configOverrides
    val admin = TestUtils.createAdminClient(brokers, listenerName, props)
    adminClients += admin
    admin
  }

  def createSuperuserAdminClient(
    listenerName: ListenerName = listenerName,
    configOverrides: Properties = new Properties
  ): Admin = {
    val props = new Properties
    props ++= superuserClientConfig
    props ++= configOverrides
    val admin = TestUtils.createAdminClient(brokers, listenerName, props)
    adminClients += admin
    admin
  }

  @AfterEach
  override def tearDown(): Unit = {
    try {
      producers.foreach(_.close(Duration.ZERO))
      consumers.foreach(_.wakeup())
      consumers.foreach(_.close(Duration.ZERO))
      shareConsumers.foreach(_.wakeup())
      shareConsumers.foreach(_.close(Duration.ZERO))
      streamsConsumers.foreach(_.wakeup())
      streamsConsumers.foreach(_.close(Duration.ZERO))
      adminClients.foreach(_.close(Duration.ZERO))

      producers.clear()
      consumers.clear()
      shareConsumers.clear()
      streamsConsumers.clear()
      adminClients.clear()
    } finally {
      super.tearDown()
    }
  }

}


Review comment (suggestion):
nit: Since `closeOptions` is a mutable object, it would be better not to reuse it.

Code after:
    props ++= adminClientConfig
    props ++= configOverrides
    val admin = TestUtils.createAdminClient(brokers, listenerName, props)
    adminClients += admin
    admin
  }

  def createSuperuserAdminClient(
    listenerName: ListenerName = listenerName,
    configOverrides: Properties = new Properties
  ): Admin = {
    val props = new Properties
    props ++= superuserClientConfig
    props ++= configOverrides
    val admin = TestUtils.createAdminClient(brokers, listenerName, props)
    adminClients += admin
    admin
  }

  @AfterEach
  override def tearDown(): Unit = {
    try {
      val closeOptions = CloseOptions.timeout(Duration.ZERO)
      producers.foreach(_.close(Duration.ZERO))
      consumers.foreach(_.wakeup())
      consumers.foreach(_.close(closeOptions))
      shareConsumers.foreach(_.wakeup())
      shareConsumers.foreach(_.close(Duration.ZERO))
      streamsConsumers.foreach(_.wakeup())
      streamsConsumers.foreach(_.close(closeOptions))
      adminClients.foreach(_.close(Duration.ZERO))

      producers.clear()
      consumers.clear()
      shareConsumers.clear()
      streamsConsumers.clear()
      adminClients.clear()
    } finally {
      super.tearDown()
    }
  }

}
Repository: apache/kafka
File: clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerBatch.java (Java)

Code before:

    public boolean hasSequence() {
        return baseSequence() != RecordBatch.NO_SEQUENCE;
    }

    public boolean isTransactional() {
        return recordsBuilder.isTransactional();
    }

    public boolean sequenceHasBeenReset() {
        return reopened;
    }

    public boolean isBufferDeallocated() {
        return bufferDeallocated;
    }

    public void markBufferDeallocated() {
        bufferDeallocated = true;
    }

    public boolean isSent() {
        return sent;
    }

    public void markSent() {
        sent = true;
    }

    // VisibleForTesting
    OptionalInt currentLeaderEpoch() {
        return currentLeaderEpoch;
    }

    // VisibleForTesting
    int attemptsWhenLeaderLastChanged() {
        return attemptsWhenLeaderLastChanged;
    }
}


Review comment (performance):
It would be useful to add a method to mark the batch as unsent when the request completes with a response. This is important since a batch could be re-enqueued for retry. It would be useful to verify in the re-enqueue test that the batch is marked as unsent after re-enqueuing. 

Code after:
    }

    public boolean hasSequence() {
        return baseSequence() != RecordBatch.NO_SEQUENCE;
    }

    public boolean isTransactional() {
        return recordsBuilder.isTransactional();
    }

    public boolean sequenceHasBeenReset() {
        return reopened;
    }

    public boolean isBufferDeallocated() {
        return bufferDeallocated;
    }

    public void markBufferDeallocated() {
        bufferDeallocated = true;
    }

    public boolean isInflight() {
        return inflight;
    }

    public void setInflight(boolean inflight) {
        this.inflight = inflight;
    }

    // VisibleForTesting
    OptionalInt currentLeaderEpoch() {
        return currentLeaderEpoch;
    }

    // VisibleForTesting
    int attemptsWhenLeaderLastChanged() {
        return attemptsWhenLeaderLastChanged;
    }
}
Repository: apache/kafka
File: streams/integration-tests/src/test/java/org/apache/kafka/streams/integration/RebalanceTaskClosureIntegrationTest.java (Java)

Code before:
            super.init(stateStoreContext, root);
        }

        @Override
        public boolean setFlushListener(final CacheFlushListener<Bytes, byte[]> listener,
                                        final boolean sendOldValues) {
            return false;
        }

        @Override
        public void flushCache() {
        }

        @Override
        public void clearCache() {
            // Clear cache is called during recycle, so we use it as a hook
            recycleLatch.countDown();
            try {
                pendingShutdownLatch.await();
            } catch (InterruptedException e) {
                throw new RuntimeException(e);
            }
        }

        @Override
        public synchronized void close() {
            closeCount.incrementAndGet();
            super.close();
        }
    }

}


Review comment (suggestion):
```suggestion
                // after we signaled via recycleLatch, that the task was converted into a "pending task",
                // we block the rebalance to complete, until we get the shutdown signal,
                // to avoid that the "pending task" get fully initialized
                // (otherwise, we don't have a pending task when the shutdown happens)
                pendingShutdownLatch.await();
```

Code after:
        public void init(final StateStoreContext stateStoreContext,
                         final StateStore root) {
            initCount.incrementAndGet();
            super.init(stateStoreContext, root);
        }

        @Override
        public boolean setFlushListener(final CacheFlushListener<Bytes, byte[]> listener,
                                        final boolean sendOldValues) {
            return false;
        }

        @Override
        public void flushCache() {
        }

        @Override
        public void clearCache() {
            // Clear cache is called during recycle, so we use it as a hook
            recycleLatch.countDown();
            try {
                // after we signaled via recycleLatch, that the task was converted into a "pending task",
                // we block the rebalance to complete, until we get the shutdown signal,
                // to avoid that the "pending task" get fully initialized
                // (otherwise, we don't have a pending task when the shutdown happens)
                pendingShutdownLatch.await();
            } catch (InterruptedException e) {
                throw new RuntimeException(e);
            }
        }

        @Override
        public synchronized void close() {
            closeCount.incrementAndGet();
            super.close();
        }
    }

}
Repository: apache/kafka
File: streams/src/main/java/org/apache/kafka/streams/state/StateSerdes.java (Java)

Code before:
        return valueSerde.deserializer();
    }

    /**
     * Return the value serializer.
     *
     * @return the value serializer
     */
    public Serializer<V> valueSerializer() {
        return valueSerde.serializer();
    }

    /**
     * Return the topic.
     *
     * @return the topic
     */
    public String topic() {
        return topic;
    }

    /**
     * Deserialize the key from raw bytes.
     *
     * @param rawKey  the key as raw bytes
     * @return        the key as typed object
     */
    @Deprecated
    public K keyFrom(final byte[] rawKey) {
        return keySerde.deserializer().deserialize(topic, rawKey);
    }

    /**
     * Deserialize the key from raw bytes.
     *
     * @param rawKey  the key as raw bytes
     * @return        the key as typed object
     */
    public K keyFrom(final byte[] rawKey, final Headers headers) {
        return keySerde.deserializer().deserialize(topic, headers, rawKey);
    }

    /**
     * Deserialize the value from raw bytes.
     *
     * @param rawValue  the value as raw bytes
     * @return          the value as typed object
     */
    @Deprecated
    public V valueFrom(final byte[] rawValue) {
        return valueSerde.deserializer().deserialize(topic, rawValue);

Review comment (suggestion):
Add:
```
@deprecated Since 4.3. Use {@link #keyFrom(byte[], Headers} instead.
```
Similar elsewhere.

Code after:
        return valueSerde.deserializer();
    }

    /**
     * Return the value serializer.
     *
     * @return the value serializer
     */
    public Serializer<V> valueSerializer() {
        return valueSerde.serializer();
    }

    /**
     * Return the topic.
     *
     * @return the topic
     */
    public String topic() {
        return topic;
    }

    /**
     * Deserialize the key from raw bytes.
     *
     * @param rawKey  the key as raw bytes
     * @return        the key as typed object
     * @deprecated Since 4.3. Use {@link #keyFrom(byte[], Headers)} instead.
     */
    @Deprecated
    public K keyFrom(final byte[] rawKey) {
        return keyFrom(rawKey, new RecordHeaders());
    }

    /**
     * Deserialize the key from raw bytes.
     *
     * @param rawKey  the key as raw bytes
     * @return        the key as typed object
     */
    public K keyFrom(final byte[] rawKey, final Headers headers) {
        return keySerde.deserializer().deserialize(topic, headers, rawKey);
    }

    /**
     * Deserialize the value from raw bytes.
     *
     * @param rawValue  the value as raw bytes
     * @return          the value as typed object
     * @deprecated Since 4.3. Use {@link #valueFrom(byte[], Headers)} instead.
     */
    @Deprecated
Repository: apache/kafka
File: streams/src/main/java/org/apache/kafka/streams/state/internals/ChangeLoggingTimestampedWindowBytesStoreWithHeaders.java (Java)

Code before:
 * <p>
 * This class extends {@link ChangeLoggingWindowBytesStore} and correctly handles
 * the header-aware storage format: [headersSize(varint)][headersBytes][timestamp(8)][value]
 * <p>
 * Unlike {@link ChangeLoggingTimestampedWindowBytesStore} which uses
 * {@link ValueAndTimestampDeserializer} for the format [timestamp(8)][value],
 * this class uses {@link ValueTimestampHeadersDeserializer} to extract
 * the timestamp from the correct position in the byte array.
 */
class ChangeLoggingTimestampedWindowBytesStoreWithHeaders extends ChangeLoggingWindowBytesStore {

    ChangeLoggingTimestampedWindowBytesStoreWithHeaders(final WindowStore<Bytes, byte[]> bytesStore,
                                                        final boolean retainDuplicates) {
        super(bytesStore, retainDuplicates, WindowKeySchema::toStoreKeyBinary);
    }

    @Override
    void log(final Bytes key,
             final byte[] valueTimestampHeaders) {
        internalContext.logChange(
            name(),
            key,
            rawValue(valueTimestampHeaders),
            valueTimestampHeaders != null ? timestamp(valueTimestampHeaders) : internalContext.recordContext().timestamp(),
            wrapped().getPosition(),
            headers(valueTimestampHeaders));
    }
}


Review comment (suggestion):
consider the case when `valueTimestampHeaders = null`

Code after:
 * <p>
 * This class extends {@link ChangeLoggingWindowBytesStore} and correctly handles
 * the header-aware storage format: [headersSize(varint)][headersBytes][timestamp(8)][value]
 * <p>
 * Unlike {@link ChangeLoggingTimestampedWindowBytesStore} which uses
 * {@link ValueAndTimestampDeserializer} for the format [timestamp(8)][value],
 * this class uses {@link ValueTimestampHeadersDeserializer} to extract
 * the timestamp from the correct position in the byte array.
 */
class ChangeLoggingTimestampedWindowBytesStoreWithHeaders extends ChangeLoggingWindowBytesStore {

    ChangeLoggingTimestampedWindowBytesStoreWithHeaders(final WindowStore<Bytes, byte[]> bytesStore,
                                                        final boolean retainDuplicates) {
        super(bytesStore, retainDuplicates, WindowKeySchema::toStoreKeyBinary);
    }

    @Override
    void log(final Bytes key,
             final byte[] valueTimestampHeaders) {
        internalContext.logChange(
            name(),
            key,
            rawValue(valueTimestampHeaders),
            valueTimestampHeaders != null
                ? timestamp(valueTimestampHeaders)
                : internalContext.recordContext().timestamp(),
            valueTimestampHeaders != null
                ? headers(valueTimestampHeaders)
                : internalContext.recordContext().headers(),
            wrapped().getPosition()
        );
    }
}
Repository: apache/superset
File: superset-frontend/src/features/alerts/AlertReportModal.tsx (TypeScript)

Code before:
            (item: { value: number; text: string }) => ({
              value: item.value,
              label: item.text,
            }),
          );
          setDashboardOptions(list);
          return { data: list, totalCount: response.json.count };
        });
      },
    [],
  );

  const getDashboardData = (db?: MetaObject) => {
    const dashboard = db || currentAlert?.dashboard;

    if (!dashboard || dashboard.label) {
      return null;
    }

    let result;

    // Cycle through dashboard options to find the selected option
    dashboardOptions.forEach(dash => {
      if (dash.value === dashboard.value || dash.value === dashboard.id) {
        result = dash;
      }
    });

    return result;
  };

  const getChartData = useCallback(
    (chartData?: MetaObject) => {
      const chart = chartData || currentAlert?.chart;

      if (!chart || chart.label) {
        return null;
      }

      let result;

      // Cycle through chart options to find the selected option
      chartOptions.forEach(slice => {
        if (slice.value === chart.value || slice.value === chart.id) {
          result = slice;
        }
      });

      return result;
    },
    [chartOptions, currentAlert?.chart],

Review comment (suggestion):
```suggestion
  }, [dashboard?.value, tabsEnabled, currentAlert?.extra]);
```

Code after:
          const list = response.json.result.map(
            (item: { value: number; text: string }) => ({
              value: item.value,
              label: item.text,
            }),
          );
          setSourceOptions(list);
          return { data: list, totalCount: response.json.count };
        });
      },
    [],
  );

  const dashboard = currentAlert?.dashboard;
  useEffect(() => {
    if (!tabsEnabled) return;

    if (dashboard?.value) {
      SupersetClient.get({
        endpoint: `/api/v1/dashboard/${dashboard.value}/tabs`,
      })
        .then(response => {
          const { tab_tree: tabTree, all_tabs: allTabs } = response.json.result;
          setTabOptions(tabTree);
          const anchor = currentAlert?.extra?.dashboard?.anchor;
          if (anchor && !(anchor in allTabs)) {
            updateAnchorState(undefined);
          }
        })
        .catch(() => {
          addDangerToast(t('There was an error retrieving dashboard tabs.'));
        });
    }
  }, [dashboard, tabsEnabled, currentAlert?.extra, addDangerToast]);

  const databaseLabel = currentAlert?.database && !currentAlert.database.label;
  useEffect(() => {
    // Find source if current alert has one set
    if (databaseLabel) {
      updateAlertState('database', getSourceData());
    }
  }, [databaseLabel, getSourceData]);

  const loadDashboardOptions = useMemo(
    () =>
      (input = '', page: number, pageSize: number) => {
        const query = rison.encode_uri({
          filter: input,
          page,
          page_size: pageSize,
        });
Repository: apache/superset
File: superset/charts/api.py (Python)

Code before:
        "is_managed_externally",
        "certified_by",
        "certification_details",
        "cache_timeout",
        "changed_by.first_name",
        "changed_by.last_name",
        "changed_by.id",
        "changed_by_name",
        "changed_on_delta_humanized",
        "changed_on_dttm",
        "changed_on_utc",
        "created_by.first_name",
        "created_by.id",
        "created_by.last_name",
        "created_by_name",
        "created_on_delta_humanized",
        "datasource_id",
        "datasource_name_text",
        "datasource_type",
        "datasource_url",
        "description",
        "description_markeddown",
        "edit_url",
        "form_data",
        "id",
        "uuid",
        "last_saved_at",
        "last_saved_by.id",
        "last_saved_by.first_name",
        "last_saved_by.last_name",
        "owners.first_name",
        "owners.id",
        "owners.last_name",
        "dashboards.id",
        "dashboards.dashboard_title",
        "params",
        "slice_name",
        "slice_url",
        "table.default_endpoint",
        "table.table_name",
        "thumbnail_url",
        "url",
        "viz_type",
        "tags.id",
        "tags.name",
        "tags.type",
        "uuid",
    ]
    list_select_columns = list_columns + ["changed_by_fk", "changed_on"]
    order_columns = [
        "changed_by.first_name",

Review comment (suggestion):
Heads up that the `uuid` field was already added to `list_columns` in https://github.com/apache/superset/pull/32414, so this would be redundant now. Not sure if this would cause it to return twice in the response, but worth double checking.

This is applicable to the other asset types too (not only to chart).

Code after:
        "certified_by",
        "certification_details",
        "cache_timeout",
        "changed_by.first_name",
        "changed_by.last_name",
        "changed_by.id",
        "changed_by_name",
        "changed_on_delta_humanized",
        "changed_on_dttm",
        "changed_on_utc",
        "created_by.first_name",
        "created_by.id",
        "created_by.last_name",
        "created_by_name",
        "created_on_delta_humanized",
        "datasource_id",
        "datasource_name_text",
        "datasource_type",
        "datasource_url",
        "description",
        "description_markeddown",
        "edit_url",
        "form_data",
        "id",
        "last_saved_at",
        "last_saved_by.id",
        "last_saved_by.first_name",
        "last_saved_by.last_name",
        "owners.first_name",
        "owners.id",
        "owners.last_name",
        "dashboards.id",
        "dashboards.dashboard_title",
        "params",
        "slice_name",
        "slice_url",
        "table.default_endpoint",
        "table.table_name",
        "thumbnail_url",
        "url",
        "viz_type",
        "tags.id",
        "tags.name",
        "tags.type",
        "uuid",
    ]
    list_select_columns = list_columns + ["changed_by_fk", "changed_on"]
    order_columns = [
        "changed_by.first_name",
        "changed_on_delta_humanized",
        "datasource_id",
Repository: apache/superset
File: tests/unit_tests/utils/csv_tests.py (Python)

Code before:
import pytest  # noqa: F401
from pandas.api.types import is_datetime64_any_dtype

from superset.utils import csv
from superset.utils.core import GenericDataType
from superset.utils.csv import (
    df_to_escaped_csv,
    get_chart_dataframe,
)


def test_escape_value():
    result = csv.escape_value("value")
    assert result == "value"

    result = csv.escape_value("-10")
    assert result == "-10"

    result = csv.escape_value("@value")
    assert result == "'@value"

    result = csv.escape_value("+value")
    assert result == "'+value"

    result = csv.escape_value("-value")
    assert result == "-value"

    result = csv.escape_value("=value")
    assert result == "'=value"

    result = csv.escape_value("|value")
    assert result == r"'\|value"

    result = csv.escape_value("%value")
    assert result == "'%value"

    result = csv.escape_value("=cmd|' /C calc'!A0")
    assert result == r"'=cmd\|' /C calc'!A0"

    result = csv.escape_value('""=10+2')
    assert result == '\'""=10+2'

    result = csv.escape_value(" =10+2")
    assert result == "' =10+2"


def fake_get_chart_csv_data_none(chart_url, auth_cookies=None):
    return None


def fake_get_chart_csv_data_empty(chart_url, auth_cookies=None):

Review comment (suggestion):
```suggestion
    assert result == "'-value"
```

Code after:
import pytest  # noqa: F401
from pandas.api.types import is_datetime64_any_dtype

from superset.utils import csv
from superset.utils.core import GenericDataType
from superset.utils.csv import (
    df_to_escaped_csv,
    get_chart_dataframe,
)


def test_escape_value():
    result = csv.escape_value("value")
    assert result == "value"

    result = csv.escape_value("-10")
    assert result == "-10"

    result = csv.escape_value("@value")
    assert result == "'@value"

    result = csv.escape_value("+value")
    assert result == "'+value"

    result = csv.escape_value("-value")
    assert result == "'-value"

    result = csv.escape_value("=value")
    assert result == "'=value"

    result = csv.escape_value("|value")
    assert result == r"'\|value"

    result = csv.escape_value("%value")
    assert result == "'%value"

    result = csv.escape_value("=cmd|' /C calc'!A0")
    assert result == r"'=cmd\|' /C calc'!A0"

    result = csv.escape_value('""=10+2')
    assert result == '\'""=10+2'

    result = csv.escape_value(" =10+2")
    assert result == "' =10+2"


def fake_get_chart_csv_data_none(chart_url, auth_cookies=None):
    return None


def fake_get_chart_csv_data_empty(chart_url, auth_cookies=None):