iffyio commented on code in PR #1435: URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1435#discussion_r1845329434
########## src/ast/helpers/ignore_field.rs: ########## @@ -0,0 +1,69 @@ +// 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. + +use core::cmp::{Eq, Ord, Ordering, PartialEq, PartialOrd}; +use core::fmt::{self, Debug, Formatter}; +use core::hash::{Hash, Hasher}; + +/// A wrapper type that ignores the field when comparing or hashing. +pub struct IgnoreField<T>(pub T); Review Comment: Instead of using a generic type would it make sense to represent via a regular wrapper type (thinking since the plan is only to wrap the tokens we attach to the nodes)? e.g. ```rust pub struct AttachedToken(TokenWithLocation); impl From<Token> for AttachedToken {} impl From<TokenWithLocation> for AttachedToken {} impl Hash for AttachedToken {} // maybe even a default/noop impl to make it easier in scenarios like test cases where the info is irrelevant // etc. pub struct With { pub with_token: AttachedToken, pub recursive: bool, // ... } ``` ########## src/ast/mod.rs: ########## @@ -140,8 +145,39 @@ pub struct Ident { /// The starting quote if any. Valid quote characters are the single quote, /// double quote, backtick, and opening square bracket. pub quote_style: Option<char>, + /// The span of the identifier in the original SQL string. + pub span: Span, +} + +impl PartialEq for Ident { + fn eq(&self, other: &Self) -> bool { + let Ident { + value, + quote_style, + // backwards compat Review Comment: ```suggestion // exhaustiveness check ``` something like that maybe? backwards compat would suggest it has to do with api changes which isn't the case? ########## src/ast/query.rs: ########## @@ -556,6 +563,8 @@ pub struct Cte { pub query: Box<Query>, pub from: Option<Ident>, pub materialized: Option<CteAsMaterialized>, + // needed for accurate span reporting Review Comment: can we update the comment to rather say what's being pointed to (thinking all attached tokens are implicitly needed for span reporting so that no need to treat this case specially)? ########## src/ast/query.rs: ########## @@ -276,6 +280,8 @@ impl fmt::Display for Table { #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] pub struct Select { + /// SELECT + pub select_token: IgnoreField<TokenWithLocation>, Review Comment: For the comment we can probably be more descriptive to mention that this points to the position of the SELECT keyword? (similar for others probably if we want to make it more obvious which part of the statement is being referred to) -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
