From codereview-return-4376-apmail-trafodion-codereview-archive=trafodion.apache.org@trafodion.incubator.apache.org Wed Feb 8 15:57:07 2017 Return-Path: X-Original-To: apmail-trafodion-codereview-archive@minotaur.apache.org Delivered-To: apmail-trafodion-codereview-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id A17D7194AE for ; Wed, 8 Feb 2017 15:57:07 +0000 (UTC) Received: (qmail 158 invoked by uid 500); 8 Feb 2017 15:57:07 -0000 Delivered-To: apmail-trafodion-codereview-archive@trafodion.apache.org Received: (qmail 99986 invoked by uid 500); 8 Feb 2017 15:57:07 -0000 Mailing-List: contact codereview-help@trafodion.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: codereview@trafodion.incubator.apache.org Delivered-To: mailing list codereview@trafodion.incubator.apache.org Received: (qmail 99795 invoked by uid 99); 8 Feb 2017 15:57:06 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd1-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 08 Feb 2017 15:57:06 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd1-us-west.apache.org (ASF Mail Server at spamd1-us-west.apache.org) with ESMTP id 5F38FC24C3 for ; Wed, 8 Feb 2017 15:57:06 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd1-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: -7.019 X-Spam-Level: X-Spam-Status: No, score=-7.019 tagged_above=-999 required=6.31 tests=[KAM_LAZY_DOMAIN_SECURITY=1, RCVD_IN_DNSWL_HI=-5, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, RP_MATCHES_RCVD=-2.999] autolearn=disabled Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd1-us-west.apache.org [10.40.0.7]) (amavisd-new, port 10024) with ESMTP id 06nGjqHOJFKf for ; Wed, 8 Feb 2017 15:57:05 +0000 (UTC) Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with SMTP id 9DDEC5F473 for ; Wed, 8 Feb 2017 15:57:04 +0000 (UTC) Received: (qmail 99776 invoked by uid 99); 8 Feb 2017 15:57:03 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 08 Feb 2017 15:57:03 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id A2530DFD9E; Wed, 8 Feb 2017 15:57:03 +0000 (UTC) From: sureshsubbiah To: codereview@trafodion.incubator.apache.org Reply-To: codereview@trafodion.incubator.apache.org References: In-Reply-To: Subject: [GitHub] incubator-trafodion pull request #946: TRAFODION-1562 Enable Vsbb upsert for... Content-Type: text/plain Message-Id: <20170208155703.A2530DFD9E@git1-us-west.apache.org> Date: Wed, 8 Feb 2017 15:57:03 +0000 (UTC) Github user sureshsubbiah commented on a diff in the pull request: https://github.com/apache/incubator-trafodion/pull/946#discussion_r100098694 --- Diff: core/sql/optimizer/BindRelExpr.cpp --- @@ -10600,34 +10600,45 @@ Upsert is also converted into merge when TRAF_UPSERT_MODE is set to MERGE and there are omitted cols with default values in case of aligned format table or omitted current timestamp cols in case of non-aligned row format */ -NABoolean Insert::isUpsertThatNeedsMerge(NABoolean isAlignedRowFormat, NABoolean omittedDefaultCols, - NABoolean omittedCurrentDefaultClassCols) const +NABoolean Insert::isUpsertThatNeedsTransformation(NABoolean isAlignedRowFormat, + NABoolean omittedDefaultCols, + NABoolean omittedCurrentDefaultClassCols, + NABoolean &toMerge) const { - // The necessary conditions to convert upsert to merge and + // The necessary conditions to transform upsert if (isUpsert() && (NOT getIsTrafLoadPrep()) && - (NOT (getTableDesc()->isIdentityColumnGeneratedAlways() && getTableDesc()->hasIdentityColumnInClusteringKey())) && + (NOT (getTableDesc()->isIdentityColumnGeneratedAlways() && + getTableDesc()->hasIdentityColumnInClusteringKey())) && (NOT (getTableDesc()->getClusteringIndex()->getNAFileSet()->hasSyskey())) && - // table has secondary indexes or - (getTableDesc()->hasSecondaryIndexes() || - // CQD is set to MERGE - ((CmpCommon::getDefault(TRAF_UPSERT_MODE) == DF_MERGE) && + // table has secondary indexes or + (getTableDesc()->hasSecondaryIndexes() )) + { + toMerge = FALSE; + return TRUE; + } + else if (isUpsert() && (NOT getIsTrafLoadPrep()) && + (NOT (getTableDesc()->isIdentityColumnGeneratedAlways() && getTableDesc()->hasIdentityColumnInClusteringKey())) && + (NOT (getTableDesc()->getClusteringIndex()->getNAFileSet()->hasSyskey())) && + // CQD is set to MERGE --- End diff -- The conditions in IF and ELSE IF are duplicated, making it harder to read. Please consider using a local variable to store the result of common conditions. Also, it is not clear what should happen if we have both IM (no syskey etc.) and default values in the upsert. With current code we will do the new efficient transformation. I suppose that means the new efficient transformation can handle the missing default values case too. Is the else if case used as fallback ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastructure@apache.org or file a JIRA ticket with INFRA. ---