Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column ...................................................................... Patch Set 12: (4 comments) http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@158 PS11, Line 158: static bool IsVirtual() { : return false; : } > nit: maybe, move this into the DataTypeTraits template itself and have cust Is that possible? I thought the DataTypeTraits template was intentionally empty; if you look at some of the other specializations, there's what appears to be intentional repetition of implementations (i.e. min_value() and max_value()). I just tried it out: I added an IsVirtual static function to the DataTypeTraits template, removed it from the specializations, but kept it in DerivedTypeTraits and DataTypeTraits<IS_DELETED>. I got the following compilation failure: FAILED: src/kudu/common/CMakeFiles/kudu_common.dir/types.cc.o /usr/lib/ccache/c++ -DHAVE_LIB_VMEM=1 -DKUDU_HEADERS_NO_STUBS=1 -DKUDU_HEADERS_USE_RICH_SLICE=1 -DKUDU_HEADERS_USE_SHORT_STATUS_MACROS=1 -DKUDU_STATIC_DEFINE -DTCMALLOC_ENABLED -D__STDC_FORMAT_MACROS -Dkudu_common_EXPORTS -Isrc -I../../src -isystem ../../thirdparty/installed/common/include -isystem ../../thirdparty/installed/uninstrumented/include -msse4.2 -Wall -Wno-sign-compare -Wno-comment -Wno-deprecated -pthread -fno-strict-aliasing -DBOOST_DATE_TIME_POSIX_TIME_STD_CONFIG -ggdb -std=c++11 -fuse-ld=gold -fsized-deallocation -g -fPIC -fPIC -MD -MT src/kudu/common/CMakeFiles/kudu_common.dir/types.cc.o -MF src/kudu/common/CMakeFiles/kudu_common.dir/types.cc.o.d -o src/kudu/common/CMakeFiles/kudu_common.dir/types.cc.o -c ../../src/kudu/common/types.cc ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)0>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)0]’ ../../src/kudu/common/types.cc:73:23: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)0>’ is_virtual_(TypeTraitsClass::IsVirtual()), ~~~~~~~~~~~~~~~~~~~~~~~~~~^~ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)1>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)1]’ ../../src/kudu/common/types.cc:74:22: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)1>’ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)2>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)2]’ ../../src/kudu/common/types.cc:75:24: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)2>’ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)3>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)3]’ ../../src/kudu/common/types.cc:76:23: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)3>’ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)4>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)4]’ ../../src/kudu/common/types.cc:77:24: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)4>’ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)5>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)5]’ ../../src/kudu/common/types.cc:78:23: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)5>’ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)6>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)6]’ ../../src/kudu/common/types.cc:79:24: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)6>’ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)7>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)7]’ ../../src/kudu/common/types.cc:80:23: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)7>’ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)9>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)9]’ ../../src/kudu/common/types.cc:83:22: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)9>’ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)10>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)10]’ ../../src/kudu/common/types.cc:84:23: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)10>’ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)11>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)11]’ ../../src/kudu/common/types.cc:85:24: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)11>’ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)12>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)12]’ ../../src/kudu/common/types.cc:86:24: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)12>’ ../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)14>]’: ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)14]’ ../../src/kudu/common/types.cc:87:24: required from here ../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)14>’ In file included from ../../src/kudu/common/types.cc:18:0: ../../src/kudu/common/types.h: In instantiation of ‘static bool kudu::DerivedTypeTraits<PhysicalType>::IsVirtual() [with kudu::DataType PhysicalType = (kudu::DataType)7]’: ../../src/kudu/common/types.cc:40:43: required from ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)13>]’ ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)13]’ ../../src/kudu/common/types.cc:81:33: required from here ../../src/kudu/common/types.h:497:51: error: ‘IsVirtual’ is not a member of ‘kudu::DataTypeTraits<(kudu::DataType)7>’ return DataTypeTraits<PhysicalType>::IsVirtual(); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~ ../../src/kudu/common/types.h: In instantiation of ‘static bool kudu::DerivedTypeTraits<PhysicalType>::IsVirtual() [with kudu::DataType PhysicalType = (kudu::DataType)12]’: ../../src/kudu/common/types.cc:40:43: required from ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)8>]’ ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)8]’ ../../src/kudu/common/types.cc:82:24: required from here ../../src/kudu/common/types.h:497:51: error: ‘IsVirtual’ is not a member of ‘kudu::DataTypeTraits<(kudu::DataType)12>’ ../../src/kudu/common/types.h: In instantiation of ‘static bool kudu::DerivedTypeTraits<PhysicalType>::IsVirtual() [with kudu::DataType PhysicalType = (kudu::DataType)5]’: ../../src/kudu/common/types.cc:40:43: required from ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)15>]’ ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)15]’ ../../src/kudu/common/types.cc:88:27: required from here ../../src/kudu/common/types.h:497:51: error: ‘IsVirtual’ is not a member of ‘kudu::DataTypeTraits<(kudu::DataType)5>’ ../../src/kudu/common/types.h: In instantiation of ‘static bool kudu::DerivedTypeTraits<PhysicalType>::IsVirtual() [with kudu::DataType PhysicalType = (kudu::DataType)14]’: ../../src/kudu/common/types.cc:40:43: required from ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)17>]’ ../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)17]’ ../../src/kudu/common/types.cc:90:28: required from here ../../src/kudu/common/types.h:497:51: error: ‘IsVirtual’ is not a member of ‘kudu::DataTypeTraits<(kudu::DataType)14>’ http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@673 PS11, Line 673: case IS_DELETED: > nit: add FALLTHROUGH_INTENDED ? Done http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@763 PS11, Line 763: // Examples: > nit: add FALLTHROUGH_INTENDED ? Done http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h@625 PS10, Line 625: return DataTypeTraits<BOOL>::name(); > But if it's name is 'bool', then how to distinguish between a column of vir It's worth adding that in normal scan operations users wouldn't necessarily see what I saw; I was working within a unit test that dumped rows. I don't feel strongly about it, so if everyone else thinks IS_DELETED (or something like it) is a better type name, I'll change it. -- To view, visit http://gerrit.cloudera.org:8080/10968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e Gerrit-Change-Number: 10968 Gerrit-PatchSet: 12 Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 02 Aug 2018 18:45:19 +0000 Gerrit-HasComments: Yes
