dtenedor commented on code in PR #48596: URL: https://github.com/apache/spark/pull/48596#discussion_r1811126029
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AnsiGetDateFieldOperationsTypeCoercion.scala: ########## @@ -0,0 +1,28 @@ +/* + * 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.spark.sql.catalyst.analysis + +import org.apache.spark.sql.catalyst.expressions.{Cast, Expression, GetDateField} +import org.apache.spark.sql.types.{AnyTimestampTypeExpression, DateType} + +object AnsiGetDateFieldOperationsTypeCoercion { Review Comment: can we please add a top level comment for each one of these new objects that mentions what it represents? (full sentences please, starting with capital letters and ending with punctuation.) For example, for this one we could just briefly mention that this contains a pattern-match against GetDateField expressions in order to return a transformed version with appropriate type casting applied. (I know the code mentions this in this particular case, but we should add a top-level comment for each object like this as a matter of procedure.) ########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AnsiTypeCoercion.scala: ########## @@ -279,31 +227,19 @@ object AnsiTypeCoercion extends TypeCoercionBase { override def transform: PartialFunction[Expression, Expression] = { // Skip nodes who's children have not been resolved yet. case g if !g.childrenResolved => g - - case g: GetDateField if AnyTimestampTypeExpression.unapply(g.child) => - g.withNewChildren(Seq(Cast(g.child, DateType))) + case withChildrenResolved + if AnsiGetDateFieldOperationsTypeCoercion.apply.isDefinedAt(withChildrenResolved) => + AnsiGetDateFieldOperationsTypeCoercion.apply(withChildrenResolved) } } object DateTimeOperations extends TypeCoercionRule { Review Comment: while we're here, can we please rename each object that inherits (directly or indirectly) from the Catalyst Rule class to end with `Rule`, to make it simpler to differentiate which of these objects are Rules and which just contain pattern-matches? -- 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]
