hughhhh commented on code in PR #43870: URL: https://github.com/apache/superset/pull/43870#discussion_r4162116308
########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/useDebouncedCommit.ts: ########## @@ -0,0 +1,85 @@ +/** + * 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. + */ +import { useCallback, useEffect, useRef, useState } from 'react'; +import { debounce } from 'lodash-es'; +import { Constants } from '@superset-ui/core/components'; + +/** + * Local text state that commits upward on a debounce. + * + * The dataset editor's commit path is asynchronous and snapshot-based: a value + * handed to `onChange` travels Field -> Fieldset -> CollectionTable -> + * DatasourceEditor -> DatasourceModal and arrives back on the `value` prop + * several renders later, and the callback that commits it was built during an + * earlier render. An input driven straight off that prop therefore loses any + * keystroke that lands while a commit is in flight -- which is why every other + * control in the editor types into local state and commits on a debounce, and + * why Fieldset's own comment says the editor assumes exactly that. This is + * TextControl's contract (src/explore/components/controls/TextControl), minus + * the ControlHeader and the number parsing this field does not want. + */ +export function useDebouncedCommit( + value: string | null | undefined, + commit: (next: string) => void, + delay: number = Constants.FAST_DEBOUNCE, +) { + const [localValue, setLocalValue] = useState(value ?? ''); + const [prevValue, setPrevValue] = useState(value); + + // The commit fires from a timer, so the callback is handed in at call time Review Comment: Fixed in `1e132aefc9`. Leftover, as you guessed — the hook never saw a monotonic flag. The claim is gone; the comment now says only why the callback is passed at call time. ########## superset/daos/dataset.py: ########## @@ -476,6 +484,48 @@ def _validate_column_date_formats( "python_date_format is an invalid date/timestamp format." ) + @staticmethod + def clear_unmapped_partition_transforms(model: SqlaTable) -> None: + """ + Drop the value transform from every column the mapping does not mirror. + + A mapping has exactly one mirrored column, so at most one column may + carry a transform. A transform parked on any other column is invisible + -- no row but the mapped one renders one -- yet it is still stored, and + it goes live the moment the mapped column resolves back to it. Clearing + an override is enough to do that: a null `partition_mapped_column` means + "follow `main_dttm_col`", so dropping a mapping would otherwise activate + whatever the default datetime column happened to be holding, turning a + request to remove a mapping into a request to add a different one. + + Enforced here rather than in the editor alone because the editor is only + one writer: a PUT, an `override_columns=true` metadata sync and an + import all reach the columns directly. The same argument + `clear_dangling_partition_mapping` makes about dangling columns. + + Gated on the feature flag, unlike its sibling, because this discards + stored configuration rather than repairing a broken reference. With the + flag off nothing mirrors, so there is no armed mapping to disarm and + clearing would be pure loss. + """ + if not feature_flag_manager.is_feature_enabled(PARTITION_FILTER_MAPPING_FLAG): + return + + mapped_column = ( + (model.partition_mapped_column or model.main_dttm_col) + if model.partition_column + else None + ) + for column in model.columns: Review Comment: Fixed in `1e132aefc9`. Confirmed, and it was the one case the method most needed to catch. `db.session.flush()` + `db.session.expire(model, ["columns"])` now run before the loop, so the collection reflects the FK inserts. Pinned by `test_a_transform_on_a_column_added_in_the_same_request_is_cleared`, which adds a column *with* a transform and points the mapping elsewhere in one request — it fails without the flush. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
